From a9c4e5fa77f7879975ef3d0b3c699928830f1c1b Mon Sep 17 00:00:00 2001 From: selul Date: Thu, 5 Nov 2026 11:16:23 +0200 Subject: [PATCH 01/11] Remove an export test run's output committed by mistake internal/cli/optimole-export-1/ (an export.json and a lock file) was left by a test run during the export work and committed with it. No test writes there any more. Co-Authored-By: Claude Opus 5.5 --- .../cli/optimole-export-1/.optimole-export.lock | 0 internal/cli/optimole-export-1/export.json | 13 ------------- 2 files changed, 13 deletions(-) delete mode 100644 internal/cli/optimole-export-1/.optimole-export.lock delete mode 100644 internal/cli/optimole-export-1/export.json diff --git a/internal/cli/optimole-export-1/.optimole-export.lock b/internal/cli/optimole-export-1/.optimole-export.lock deleted file mode 100644 index e69de29..0000000 diff --git a/internal/cli/optimole-export-1/export.json b/internal/cli/optimole-export-1/export.json deleted file mode 100644 index ed7a578..0000000 --- a/internal/cli/optimole-export-1/export.json +++ /dev/null @@ -1,13 +0,0 @@ -{ - "id": 1, - "status": "completed", - "created_at": "2026-11-04T10:01:00.000000Z", - "domains": [ - { - "domain": "directUpload", - "folder": "directUpload", - "status": "completed", - "archives": [] - } - ] -} From 5fc88dcb14e6e4acc2017b83d09d9ad052e30d34 Mon Sep 17 00:00:00 2001 From: selul Date: Thu, 5 Nov 2026 11:16:23 +0200 Subject: [PATCH 02/11] api: look an asset up by its key GetAsset calls GET /api/assets/{key}, which answers with the asset's CDN URL, MIME type and original size. The dashboard answers 403 both for an unknown key and for an asset a folder-scoped key may not see. The fake server serves the route the same way, and AddAsset puts a finished asset in its library. Co-Authored-By: Claude Opus 5.5 --- internal/api/client.go | 28 +++++++++--- internal/api/client_test.go | 64 +++++++++++++++++++++++++- internal/api/types.go | 3 ++ internal/testserver/testserver.go | 75 ++++++++++++++++++++++++++++++- 4 files changed, 160 insertions(+), 10 deletions(-) diff --git a/internal/api/client.go b/internal/api/client.go index 394556e..a496aac 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -1,7 +1,7 @@ // Package api is a small client for the parts of the Optimole dashboard API // the CLI uses: reserving an asset, sending its bytes to the presigned storage -// URL, completing it, listing folders, and running asset exports and -// downloading their archives. +// URL, completing it, looking an asset up, listing folders, and running asset +// exports and downloading their archives. package api import ( @@ -42,6 +42,7 @@ const ( const ( OpCreateAsset = "create asset" OpCompleteUpload = "complete upload" + OpGetAsset = "get asset" OpListFolders = "list folders" ) @@ -286,18 +287,31 @@ func (c *Client) CompleteAsset(ctx context.Context, key string, dims *Dimensions if err := c.doJSON(ctx, OpCompleteUpload, http.MethodPost, c.endpoint("assets", strings.ToLower(key), "completion"), body, &raw, retryIdempotent); err != nil { return nil, err } - return decodeAsset(raw) + return decodeAsset(OpCompleteUpload, raw) +} + +// GetAsset looks an asset up by its key. The dashboard answers 403 both for +// a key it does not know and for an asset the API key may not see. +func (c *Client) GetAsset(ctx context.Context, key string) (*Asset, error) { + if !assetKeyPattern.MatchString(key) { + return nil, fmt.Errorf("%s: invalid asset key %q", OpGetAsset, key) + } + var raw json.RawMessage + if err := c.doJSON(ctx, OpGetAsset, http.MethodGet, c.endpoint("assets", strings.ToLower(key)), nil, &raw, retryIdempotent); err != nil { + return nil, err + } + return decodeAsset(OpGetAsset, raw) } // decodeAsset accepts a bare AssetResource and, defensively, one wrapped in -// "asset" or "data". -func decodeAsset(raw json.RawMessage) (*Asset, error) { +// "asset" or "data". op names the call in errors. +func decodeAsset(op string, raw json.RawMessage) (*Asset, error) { var wrapped struct { Asset *Asset `json:"asset"` Data *Asset `json:"data"` } if err := json.Unmarshal(raw, &wrapped); err != nil { - return nil, fmt.Errorf("complete upload: invalid JSON response: %w", err) + return nil, fmt.Errorf("%s: invalid JSON response: %w", op, err) } if wrapped.Asset != nil { return wrapped.Asset, nil @@ -307,7 +321,7 @@ func decodeAsset(raw json.RawMessage) (*Asset, error) { } var a Asset if err := json.Unmarshal(raw, &a); err != nil { - return nil, fmt.Errorf("complete upload: invalid JSON response: %w", err) + return nil, fmt.Errorf("%s: invalid JSON response: %w", op, err) } return &a, nil } diff --git a/internal/api/client_test.go b/internal/api/client_test.go index 4ee6f43..af85718 100644 --- a/internal/api/client_test.go +++ b/internal/api/client_test.go @@ -667,7 +667,7 @@ func TestDecodeAssetShapes(t *testing.T) { `{"asset":{"resourceS3":"k","url":"https://x/u","folder_id":"12"}}`, `{"data":{"resourceS3":"k","url":"https://x/u","folder_id":12}}`, } { - a, err := decodeAsset([]byte(body)) + a, err := decodeAsset(OpCompleteUpload, []byte(body)) if err != nil || a.URL != "https://x/u" || a.FolderID.Value != 12 { t.Errorf("decodeAsset(%s) = %+v, %v", body, a, err) } @@ -750,3 +750,65 @@ func TestBackoffIsCapped(t *testing.T) { } } } + +func TestGetAsset(t *testing.T) { + srv := testserver.New(t) + c, sl := newTestClient(t, srv.URL) + ctx := context.Background() + key := srv.AddAsset(testserver.Asset{Filename: "photo.jpg", ContentType: "image/jpeg", Size: 9, Width: 1200, Height: 800}) + + a, err := c.GetAsset(ctx, strings.ToUpper(key)) + if err != nil { + t.Fatal(err) + } + want := "https://app.i.optimole.com/w:auto/h:auto/q:auto/id:" + key + "/directUpload/photo.jpg" + if a.Key != key || a.URL != want || a.MimeType != "image/jpeg" || a.Width.Value != 1200 || a.Height.Value != 800 { + t.Fatalf("asset = %+v", a) + } + if ev := srv.Events()[0]; ev.Route != testserver.RouteAsset || ev.Key != key { + t.Fatalf("event = %+v", ev) + } + + // An unknown key is a 403, which is final. + _, err = c.GetAsset(ctx, strings.Repeat("ab", 16)) + var apiErr *Error + if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusForbidden || apiErr.Op != OpGetAsset || srv.Count(testserver.RouteAsset) != 2 { + t.Fatalf("err = %v, %d calls", err, srv.Count(testserver.RouteAsset)) + } + + // A busy dashboard is retried: the lookup is safe to repeat. + srv.Inject(testserver.RouteAsset, testserver.Response{Status: http.StatusServiceUnavailable, Body: `{"message":"Busy."}`}) + if _, err := c.GetAsset(ctx, key); err != nil || srv.Count(testserver.RouteAsset) != 4 || len(sl.all()) != 1 { + t.Fatalf("err = %v, %d calls, %d waits", err, srv.Count(testserver.RouteAsset), len(sl.all())) + } + + sent := len(srv.Events()) + for _, bad := range []string{"", "../../me", key + "0", "zz" + key[2:]} { + if _, err := c.GetAsset(ctx, bad); err == nil { + t.Fatalf("GetAsset(%q) succeeded", bad) + } + } + if n := len(srv.Events()); n != sent { + t.Fatalf("invalid keys sent %d requests", n-sent) + } +} + +func TestGetAssetHonoursFolderScopedKeys(t *testing.T) { + srv := testserver.New(t) + shared := srv.AddFolder("Shared") + other := srv.AddFolder("Other") + srv.AddKey("optml-s-scoped", testserver.Key{Scoped: true, Granted: []int64{shared}}) + inShared := srv.AddAsset(testserver.Asset{Filename: "a.jpg", ContentType: "image/jpeg", FolderID: shared}) + inOther := srv.AddAsset(testserver.Asset{Filename: "b.jpg", ContentType: "image/jpeg", FolderID: other}) + c, err := New(Options{BaseURL: srv.URL, APIKey: "optml-s-scoped", UserAgent: "optimole-cli/test"}) + if err != nil { + t.Fatal(err) + } + if _, err := c.GetAsset(context.Background(), inShared); err != nil { + t.Fatalf("asset in a shared folder: %v", err) + } + var apiErr *Error + if _, err := c.GetAsset(context.Background(), inOther); !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusForbidden { + t.Fatalf("asset outside the shared folders: %v", err) + } +} diff --git a/internal/api/types.go b/internal/api/types.go index efda8f9..5039764 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -26,6 +26,9 @@ type Asset struct { Status string `json:"itemStatus"` MimeType string `json:"mimeType"` URL string `json:"url"` + // Width and Height are the original image's size in pixels, when known. + Width FlexInt `json:"originalWidth"` + Height FlexInt `json:"originalHeight"` } // UploadInstructions tells the client how to send the file bytes to storage. diff --git a/internal/testserver/testserver.go b/internal/testserver/testserver.go index 4e399a1..3675dde 100644 --- a/internal/testserver/testserver.go +++ b/internal/testserver/testserver.go @@ -38,6 +38,7 @@ const ( RouteCreate Route = "create" RoutePut Route = "put" RouteComplete Route = "complete" + RouteAsset Route = "asset" RouteFolders Route = "folders" ) @@ -92,6 +93,9 @@ type Asset struct { Width int Height int HadHints bool + // NoURL makes the asset's answer carry "url": null, as the dashboard + // does for a file that is not on the CDN yet. + NoURL bool } // Event records one request the fake handled, with sequence numbers taken @@ -173,7 +177,7 @@ func New(t testing.TB) *Server { } api := http.NewServeMux() api.HandleFunc("/api/assets", s.handleCreate) - api.HandleFunc("/api/assets/", s.handleComplete) + api.HandleFunc("/api/assets/", s.handleAssetPath) api.HandleFunc("/api/folders", s.handleFolders) api.HandleFunc("/api/asset-exports", s.handleExports) api.HandleFunc("/api/asset-exports/", s.handleExport) @@ -581,6 +585,69 @@ func firstNonEmpty(cond bool, a, b string) string { var completionPath = regexp.MustCompile(`^/api/assets/([0-9a-fA-F]{32})/completion$`) +// AddAsset puts a finished asset in the library, as if it had been uploaded, +// and returns its key (a random one when a.Key is empty). +func (s *Server) AddAsset(a Asset) string { + if a.Key == "" { + a.Key = randomKey() + } + a.Key = strings.ToLower(a.Key) + a.Uploaded, a.Completed, a.UploadedSize = true, true, a.Size + s.mu.Lock() + defer s.mu.Unlock() + s.assets[a.Key] = &a + return a.Key +} + +var assetPath = regexp.MustCompile(`^/api/assets/([0-9a-fA-F]{32})$`) + +// handleAssetPath sends GET /assets/{key} to handleGetAsset and everything +// else under /assets/ to handleComplete. +func (s *Server) handleAssetPath(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && assetPath.MatchString(r.URL.Path) { + s.handleGetAsset(w, r) + return + } + s.handleComplete(w, r) +} + +// handleGetAsset answers GET /assets/{key} like the dashboard: 403 both for +// a key it does not know and for an asset outside a scoped key's folders. +func (s *Server) handleGetAsset(w http.ResponseWriter, r *http.Request) { + ev, injected := s.begin(RouteAsset, r) + status := 0 + var drop func() + defer func() { s.finishThenDrop(ev, status, drop) }() + key := strings.ToLower(assetPath.FindStringSubmatch(r.URL.Path)[1]) + ev.Key = key + if injected != nil { + status, drop = writeInjected(w, injected) + return + } + if key != assetPath.FindStringSubmatch(r.URL.Path)[1] { + s.violate("GET %s: the key is not lower case", r.URL.Path) + } + k, ok := s.authenticate(w, r) + if !ok { + status = http.StatusUnauthorized + return + } + s.mu.Lock() + a := s.assets[key] + var resp map[string]any + var folderID int64 + if a != nil && a.Completed { + resp = s.assetJSONLocked(a) + folderID = a.FolderID + } + s.mu.Unlock() + if resp == nil || (k.Scoped && !s.granted(k, folderID)) { + status = writeJSON(w, http.StatusForbidden, map[string]string{"message": "This action is unauthorized."}) + return + } + status = writeJSON(w, http.StatusOK, resp) +} + func (s *Server) handleComplete(w http.ResponseWriter, r *http.Request) { ev, injected := s.begin(RouteComplete, r) status := 0 @@ -666,10 +733,14 @@ func (s *Server) assetJSONLocked(a *Asset) map[string]any { if a.FolderID > 0 { folder = a.FolderID } + var u any = url + if a.NoURL { + u = nil + } return map[string]any{ "resourceS3": a.Key, "folder_id": folder, "itemStatus": "done", "uploaded_status": 1, "mimeType": a.ContentType, "fileSize": a.Size, "originalWidth": a.Width, "originalHeight": a.Height, - "url": url, + "url": u, } } From 4f8078bbd0f9799da5e3464bc711cb7a3cdad605 Mon Sep 17 00:00:00 2001 From: selul Date: Thu, 5 Nov 2026 11:16:23 +0200 Subject: [PATCH 03/11] imgurl: read and rewrite Optimole URLs the way the CDN does Splits a delivery URL into its options and source (id:HASH/... or an origin URL), following the CDN's viewer-request function: only the first copy of an option counts, and options are applied in a fixed order, which String writes them in. Check refuses values the CDN answers with an empty image (w, h, q), ignores (a cache buster without a dot) or resets (an unknown gravity). AVIF cannot be forced, because the CDN drops f:avif, and the resize types are the dashboard's (rt:crop answers a redirect). Co-Authored-By: Claude Opus 5.5 --- internal/imgurl/imgurl.go | 232 ++++++++++++++++++++++++++++++ internal/imgurl/imgurl_test.go | 243 ++++++++++++++++++++++++++++++++ internal/imgurl/options.go | 243 ++++++++++++++++++++++++++++++++ internal/imgurl/options_test.go | 99 +++++++++++++ 4 files changed, 817 insertions(+) create mode 100644 internal/imgurl/imgurl.go create mode 100644 internal/imgurl/imgurl_test.go create mode 100644 internal/imgurl/options.go create mode 100644 internal/imgurl/options_test.go diff --git a/internal/imgurl/imgurl.go b/internal/imgurl/imgurl.go new file mode 100644 index 0000000..a74926f --- /dev/null +++ b/internal/imgurl/imgurl.go @@ -0,0 +1,232 @@ +// Package imgurl reads and rewrites Optimole delivery URLs: +// +// https://HOST/OPTION/OPTION/.../SOURCE +// +// SOURCE is "id:HASH/ORIGIN/FILE" for library assets, or the origin URL +// itself ("https://example.com/a.jpg") for images the CDN fetches from a +// site. OPTION segments are "name:value" (w:800, q:auto, rt:fill). +// +// The package reads a path the way the CDN's viewer-request function does: +// only the first copy of an option counts, and the CDN applies options in a +// fixed order whatever order the path lists them in. String writes them in +// that order, so equal options always give the same URL. +package imgurl + +import ( + "errors" + "fmt" + "strings" +) + +// ErrNotOptimole is returned for URLs that are not Optimole delivery URLs. +var ErrNotOptimole = errors.New("not an Optimole URL") + +// Kind tells images from other files. +type Kind int + +const ( + // Image is an image the CDN can transform. + Image Kind = iota + // File is any other file: a library file ("/files/" in the URL) or a + // CSS or JS asset ("f:css", "f:js"). Image options do not apply to it. + File +) + +// Option is one "name:value" path segment. Value keeps any further colons +// ("fp:0.5:0.2" for g). +type Option struct { + Name string + Value string +} + +// String returns the path segment. +func (o Option) String() string { return o.Name + ":" + o.Value } + +// URL is an Optimole delivery URL split into the parts the CDN reads. +type URL struct { + // prefix is the scheme and host, e.g. "https://abc.i.optimole.com". + prefix string + // options are the path's options in path order. A name can repeat: Get, + // Set and Options use its first copy, the one the CDN applies, and + // Unset removes every copy. + options []Option + // extra are the other segments before the source, kept as they are: + // "files" marks a file URL, and the CDN ignores the rest. + extra []string + // source is the rest of the path, from "id:" or the origin's scheme. + source string + // suffix is the query and fragment, kept as they are. + suffix string +} + +// defaultImageOptions are the options of an image URL from the dashboard. +var defaultImageOptions = []Option{{"w", "auto"}, {"h", "auto"}, {"q", "auto"}} + +// Parse splits an Optimole URL. The host must be an Optimole one, or the +// path must hold an "id:" source or CDN options (custom domains). +func Parse(raw string) (*URL, error) { + s := strings.TrimSpace(raw) + scheme, rest, ok := cutScheme(s) + if !ok { + return nil, fmt.Errorf("%w: it must start with https://", ErrNotOptimole) + } + host, path, _ := strings.Cut(rest, "/") + if host == "" || strings.ContainsAny(host, "@?#\\ \t") { + return nil, fmt.Errorf("%w: invalid host %q", ErrNotOptimole, host) + } + path, suffix := cutSuffix(path) + // The CDN reads an escaped colon only when the path has no plain one. + if !strings.Contains(path, ":") { + path = strings.NewReplacer("%3A", ":", "%3a", ":").Replace(path) + } + segs := strings.Split(path, "/") + start := sourceStart(segs) + if start < 0 { + return nil, fmt.Errorf("%w: the path has no id: or origin URL", ErrNotOptimole) + } + u := &URL{prefix: scheme + "://" + host, source: strings.Join(segs[start:], "/"), suffix: suffix} + for _, seg := range segs[:start] { + if seg == "" { + continue + } + name, value, isOption := strings.Cut(seg, ":") + if isOption && IsOption(name) { + u.options = append(u.options, Option{name, value}) + continue + } + u.extra = append(u.extra, seg) + } + if !isOptimoleHost(host) && len(u.options) == 0 && u.ID() == "" { + return nil, fmt.Errorf("%w: %s is not an Optimole host and the path has no CDN options", ErrNotOptimole, host) + } + return u, nil +} + +// cutScheme splits "https://rest" (any case) and returns the scheme in +// lower case. +func cutScheme(s string) (scheme, rest string, ok bool) { + for _, sc := range []string{"https", "http"} { + if len(s) > len(sc)+3 && strings.EqualFold(s[:len(sc)+3], sc+"://") { + return sc, s[len(sc)+3:], true + } + } + return "", "", false +} + +// cutSuffix splits the path from its query and fragment. +func cutSuffix(path string) (string, string) { + if i := strings.IndexAny(path, "?#"); i >= 0 { + return path[:i], path[i:] + } + return path, "" +} + +// sourceStart returns the index of the first "id:HASH" segment, or of the +// "https:" that starts an origin URL ("https:", "", host), or -1. +func sourceStart(segs []string) int { + for i, seg := range segs { + if len(seg) > len("id:") && strings.HasPrefix(seg, "id:") { + return i + } + if (seg == "https:" || seg == "http:") && i+2 < len(segs) && segs[i+1] == "" && segs[i+2] != "" { + return i + } + } + return -1 +} + +func isOptimoleHost(host string) bool { + h := strings.ToLower(host) + return strings.HasSuffix(h, ".i.optimole.com") || strings.HasSuffix(h, ".optml.cloud") +} + +// ID returns the asset hash of an "id:" URL, or "". +func (u *URL) ID() string { + rest, ok := strings.CutPrefix(u.source, "id:") + if !ok { + return "" + } + id, _, _ := strings.Cut(rest, "/") + return id +} + +// Kind tells whether the URL is an image the CDN can transform. +func (u *URL) Kind() Kind { + for _, e := range u.extra { + if e == "files" { + return File + } + } + if f, ok := u.Get("f"); ok && (f == "css" || f == "js") { + return File + } + return Image +} + +// Get returns the value of an option. +func (u *URL) Get(name string) (string, bool) { + for _, o := range u.options { + if o.Name == name { + return o.Value, true + } + } + return "", false +} + +// Set adds an option or replaces its value. +func (u *URL) Set(name, value string) { + for i := range u.options { + if u.options[i].Name == name { + u.options[i].Value = value + return + } + } + u.options = append(u.options, Option{name, value}) +} + +// Unset removes an option. +func (u *URL) Unset(name string) { + kept := u.options[:0] + for _, o := range u.options { + if o.Name != name { + kept = append(kept, o) + } + } + u.options = kept +} + +// Reset replaces the options with those of a dashboard image URL +// (w:auto/h:auto/q:auto). +func (u *URL) Reset() { + u.options = append([]Option(nil), defaultImageOptions...) +} + +// Options returns one option per name, in the order String writes them. +func (u *URL) Options() []Option { + sorted := make([]Option, 0, len(u.options)) + for _, name := range cdnOptions { + if v, ok := u.Get(name); ok { + sorted = append(sorted, Option{name, v}) + } + } + return sorted +} + +// String returns the URL with its options in the CDN's order, followed by +// the other segments and the source. +func (u *URL) String() string { + var b strings.Builder + b.WriteString(u.prefix) + for _, o := range u.Options() { + b.WriteByte('/') + b.WriteString(o.String()) + } + for _, e := range u.extra { + b.WriteByte('/') + b.WriteString(e) + } + b.WriteByte('/') + b.WriteString(u.source) + b.WriteString(u.suffix) + return b.String() +} diff --git a/internal/imgurl/imgurl_test.go b/internal/imgurl/imgurl_test.go new file mode 100644 index 0000000..4c30e0c --- /dev/null +++ b/internal/imgurl/imgurl_test.go @@ -0,0 +1,243 @@ +package imgurl + +import ( + "errors" + "reflect" + "testing" +) + +const ( + hash = "42cf6621995747f69a30d32f6a6c4f82" + libImage = "https://mlabc.i.optimole.com/w:auto/h:auto/q:auto/id:" + hash + "/directUpload/photo.jpg" + libFile = "https://mlabc.i.optimole.com/files/id:" + hash + "/directUpload/notes.pdf" +) + +func mustParse(t *testing.T, raw string) *URL { + t.Helper() + u, err := Parse(raw) + if err != nil { + t.Fatalf("Parse(%q): %v", raw, err) + } + return u +} + +func TestParseKeepsDashboardURLs(t *testing.T) { + for _, raw := range []string{ + libImage, + libFile, + "https://mlabc.i.optimole.com/w:auto/h:auto/q:auto/id:" + hash + "/https://example.com/a.jpg", + "https://mlabc.b.optml.cloud/w:auto/h:auto/q:auto/id:" + hash + "/directUpload/photo.jpg", + } { + if got := mustParse(t, raw).String(); got != raw { + t.Errorf("String() = %q, want the input %q", got, raw) + } + } +} + +func TestParseReadsPartsLikeTheCDN(t *testing.T) { + tests := []struct { + name, raw string + options []Option + id string + kind Kind + out string + }{ + { + name: "library image", + raw: libImage, + options: []Option{{"w", "auto"}, {"h", "auto"}, {"q", "auto"}}, + id: hash, + kind: Image, + out: libImage, + }, + { + name: "library file", + raw: libFile, + id: hash, + kind: File, + out: libFile, + }, + { + name: "WordPress plugin URL: options sorted into the CDN's order", + raw: "https://mlabc.i.optimole.com/cb:aB3d.1f/w:800/h:600/f:best/ig:avif/q:mauto/rt:fill/g:ce/https://example.com/wp-content/uploads/a.jpg", + options: []Option{{"cb", "aB3d.1f"}, {"w", "800"}, {"h", "600"}, {"q", "mauto"}, {"rt", "fill"}, {"g", "ce"}, {"f", "best"}, {"ig", "avif"}}, + kind: Image, + out: "https://mlabc.i.optimole.com/cb:aB3d.1f/w:800/h:600/q:mauto/rt:fill/g:ce/f:best/ig:avif/https://example.com/wp-content/uploads/a.jpg", + }, + { + name: "a repeated option: the first copy counts, as on the CDN", + raw: "https://mlabc.i.optimole.com/w:800/w:400/h:auto/id:" + hash + "/directUpload/a.jpg", + options: []Option{{"w", "800"}, {"h", "auto"}}, + id: hash, + kind: Image, + out: "https://mlabc.i.optimole.com/w:800/h:auto/id:" + hash + "/directUpload/a.jpg", + }, + { + name: "segments the CDN does not read are kept after the options", + raw: "https://mlabc.i.optimole.com/wdth:300/w:200/extra/id:" + hash + "/directUpload/a.jpg", + options: []Option{{"w", "200"}}, + id: hash, + kind: Image, + out: "https://mlabc.i.optimole.com/w:200/wdth:300/extra/id:" + hash + "/directUpload/a.jpg", + }, + { + name: "escaped colons are read when the path has no plain one", + raw: "https://mlabc.i.optimole.com/w%3A300/id%3A" + hash + "/directUpload/a.jpg", + options: []Option{{"w", "300"}}, + id: hash, + kind: Image, + out: "https://mlabc.i.optimole.com/w:300/id:" + hash + "/directUpload/a.jpg", + }, + { + name: "query and fragment are kept; the origin's own path is untouched", + raw: "https://mlabc.i.optimole.com/w:300/https://example.com/w:9/a.jpg?ver=2#top", + options: []Option{{"w", "300"}}, + kind: Image, + out: "https://mlabc.i.optimole.com/w:300/https://example.com/w:9/a.jpg?ver=2#top", + }, + { + name: "a CSS asset is a file", + raw: "https://mlabc.i.optimole.com/cb:aB3d.1f/f:css/q:mauto/m:0/https://example.com/style.css", + options: []Option{{"cb", "aB3d.1f"}, {"q", "mauto"}, {"f", "css"}, {"m", "0"}}, + kind: File, + out: "https://mlabc.i.optimole.com/cb:aB3d.1f/q:mauto/f:css/m:0/https://example.com/style.css", + }, + { + name: "a custom domain with an id: source", + raw: "https://images.example.com/w:auto/h:auto/q:auto/id:" + hash + "/directUpload/a.jpg", + options: []Option{{"w", "auto"}, {"h", "auto"}, {"q", "auto"}}, + id: hash, + kind: Image, + out: "https://images.example.com/w:auto/h:auto/q:auto/id:" + hash + "/directUpload/a.jpg", + }, + { + name: "a custom domain with CDN options and an origin URL", + raw: "https://cdn.example.com/w:640/https://example.com/a.jpg", + options: []Option{{"w", "640"}}, + kind: Image, + out: "https://cdn.example.com/w:640/https://example.com/a.jpg", + }, + { + name: "upper-case scheme", + raw: "HTTPS://mlabc.i.optimole.com/id:" + hash + "/directUpload/a.jpg", + id: hash, + kind: Image, + out: "https://mlabc.i.optimole.com/id:" + hash + "/directUpload/a.jpg", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + u := mustParse(t, tt.raw) + if got := u.Options(); len(got)+len(tt.options) > 0 && !reflect.DeepEqual(got, tt.options) { + t.Errorf("Options() = %v, want %v", got, tt.options) + } + if u.ID() != tt.id { + t.Errorf("ID() = %q, want %q", u.ID(), tt.id) + } + if u.Kind() != tt.kind { + t.Errorf("Kind() = %v, want %v", u.Kind(), tt.kind) + } + if s := u.String(); s != tt.out { + t.Errorf("String() = %q, want %q", s, tt.out) + } + }) + } +} + +func TestParseRefusesOtherURLs(t *testing.T) { + for _, raw := range []string{ + "", + hash, + "ftp://mlabc.i.optimole.com/id:" + hash + "/a.jpg", + "https://", + "https://user@mlabc.i.optimole.com/id:" + hash + "/a.jpg", + "https://mlabc.i.optimole.com/w:300/a.jpg", + "https://mlabc.i.optimole.com/w:300/https:/example.com/a.jpg", + "https://mlabc.i.optimole.com/id:/directUpload/a.jpg", + "https://web.archive.org/web/2020/https://example.com/a.jpg", + "https://example.com/images/https://example.org/a.jpg", + } { + if u, err := Parse(raw); !errors.Is(err, ErrNotOptimole) { + t.Errorf("Parse(%q) = %v, %v; want ErrNotOptimole", raw, u, err) + } + } +} + +func TestSetReplacesInPlace(t *testing.T) { + u := mustParse(t, libImage) + u.Set("w", "300") + u.Set("f", "webp") + want := "https://mlabc.i.optimole.com/w:300/h:auto/q:auto/f:webp/id:" + hash + "/directUpload/photo.jpg" + if got := u.String(); got != want { + t.Fatalf("String() = %q, want %q", got, want) + } + // Setting it again replaces the value, never adds a second copy (the CDN + // would keep the first). + u.Set("w", "200") + if got, _ := u.Get("w"); got != "200" || len(u.Options()) != 4 { + t.Fatalf("after a second Set: w=%q, options %v", got, u.Options()) + } +} + +func TestUnsetAndReset(t *testing.T) { + u := mustParse(t, "https://mlabc.i.optimole.com/w:300/h:200/q:60/rt:fill/f:webp/id:"+hash+"/directUpload/a.jpg") + u.Unset("f") + u.Unset("missing") + if got, want := u.String(), "https://mlabc.i.optimole.com/w:300/h:200/q:60/rt:fill/id:"+hash+"/directUpload/a.jpg"; got != want { + t.Fatalf("after Unset: %q, want %q", got, want) + } + u.Reset() + if got, want := u.String(), "https://mlabc.i.optimole.com/w:auto/h:auto/q:auto/id:"+hash+"/directUpload/a.jpg"; got != want { + t.Fatalf("after Reset: %q, want %q", got, want) + } + // Reset gives the URL its own options: changing them leaves the defaults + // alone for the next URL. + u.Set("w", "10") + v := mustParse(t, libImage) + v.Reset() + if got, _ := v.Get("w"); got != "auto" { + t.Fatalf("defaults changed by an earlier Set: w=%q", got) + } +} + +func TestStringReparsesToTheSameURL(t *testing.T) { + for _, raw := range []string{ + libImage, + "https://mlabc.i.optimole.com/q:60/zz/w:5/w:6/files/https://example.com/a.css?x=1", + "https://cdn.example.com/g:fp:0.5:0.1/rt:fill/https://example.com/a.jpg", + } { + first := mustParse(t, raw).String() + if second := mustParse(t, first).String(); second != first { + t.Errorf("String() is not stable: %q then %q", first, second) + } + } +} + +func FuzzParse(f *testing.F) { + for _, seed := range []string{ + libImage, + libFile, + "https://mlabc.i.optimole.com/cb:aB3d.1f/w:800/f:best/ig:avif/https://example.com/a.jpg?v=1", + "https://cdn.example.com/w%3A1/id%3Ax/y", + "https://x.optml.cloud//w:1//id:a/b", + } { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, raw string) { + u, err := Parse(raw) + if err != nil { + return + } + s := u.String() + v, err := Parse(s) + if err != nil { + t.Fatalf("String() of %q is %q, which does not parse: %v", raw, s, err) + } + if v.String() != s { + t.Fatalf("not stable: %q -> %q -> %q", raw, s, v.String()) + } + if !reflect.DeepEqual(u.Options(), v.Options()) || u.ID() != v.ID() || u.Kind() != v.Kind() { + t.Fatalf("reparsing %q changed it: %v %q %v -> %v %q %v", s, u.Options(), u.ID(), u.Kind(), v.Options(), v.ID(), v.Kind()) + } + }) +} diff --git a/internal/imgurl/options.go b/internal/imgurl/options.go new file mode 100644 index 0000000..981509a --- /dev/null +++ b/internal/imgurl/options.go @@ -0,0 +1,243 @@ +package imgurl + +import ( + "errors" + "fmt" + "strconv" + "strings" + "unicode" +) + +// cdnOptions are the options the CDN reads, in the order it applies them: +// its viewer-request function sorts a path's options into this order and +// drops any other "name:value" segment before the source. "ig" (only +// "ig:avif": leave AVIF out of the formats a browser can get) is read before +// that sort, so its place does not matter. "id" is not here: it starts the +// source. +var cdnOptions = []string{ + "cb", "c", "w", "h", "q", "rt", "rs", "g", "dpr", "f", "ig", "bl", "sh", + "pix", "br", "co", "sa", "process", "m", "mh", "mw", "zoom", "el", "ex", + "pd", "trim", "rot", "ar", "bg", "fq", "autoquality", "mb", "jpgo", "pngo", + "pg", "vts", "fiu", "skp", "exp", "fn", "pr", "strip_metadata", "sm", + "keep_copyright", "scp", "eth", "att", "wm", "wmu", "wmt", "wms", "st", + "no-auto", "raw", +} + +// IsOption reports whether the CDN reads an option of this name. +func IsOption(name string) bool { + for _, n := range cdnOptions { + if n == name { + return true + } + } + return false +} + +// Check validates an option the way the CDN reads it. Options the CDN +// answers with an empty image for (bad w, h or q), silently drops (a cache +// buster without a dot) or resets (an unknown gravity) are refused. +func Check(name, value string) error { + if !IsOption(name) { + return fmt.Errorf("%q is not an option the CDN reads", name) + } + if value == "" { + return errors.New("missing value") + } + if strings.ContainsFunc(value, func(r rune) bool { + return r == '/' || r == '?' || r == '#' || r == '%' || unicode.IsSpace(r) || unicode.IsControl(r) + }) { + return errors.New("the value cannot contain /, ?, #, % or spaces") + } + var err error + switch name { + case "w", "h": + _, err = Size(value) + case "q": + _, err = Quality(value) + case "rt": + _, err = ResizeType(value) + case "g": + err = checkGravityValue(value) + case "dpr": + _, err = DPR(value) + case "cb": + err = checkCacheBuster(value) + } + return err +} + +// Size validates a width or height: "auto" or a number of pixels. +func Size(v string) (string, error) { + if v == "auto" { + return v, nil + } + n, err := strconv.ParseInt(v, 10, 32) + if err != nil || n < 1 || strings.HasPrefix(v, "+") || strings.HasPrefix(v, "0") { + return "", errors.New(`must be "auto" or a number of pixels (1 or more)`) + } + return v, nil +} + +// Quality validates a quality: auto, mauto, eco or 1-100. +func Quality(v string) (string, error) { + switch v { + case "auto", "mauto", "eco": + return v, nil + } + n, err := strconv.Atoi(v) + if err != nil || n < 1 || n > 100 || strings.HasPrefix(v, "+") || strings.HasPrefix(v, "0") { + return "", errors.New("must be auto, mauto, eco or a number from 1 to 100") + } + return v, nil +} + +// resizeTypes are the resize types the dashboard offers. The CDN answers +// "crop" with a redirect to the original image. +var resizeTypes = []string{"fit", "fill", "fill-down", "force", "auto"} + +// ResizeType validates a resize type. +func ResizeType(v string) (string, error) { + for _, t := range resizeTypes { + if v == t { + return v, nil + } + } + return "", fmt.Errorf("must be %s", list(resizeTypes)) +} + +// gravityWords maps the names the CLI takes to the CDN's gravity codes. +var gravityWords = []struct{ word, code string }{ + {"center", "ce"}, + {"top", "no"}, + {"bottom", "so"}, + {"left", "we"}, + {"right", "ea"}, + {"top-left", "nowe"}, + {"top-right", "noea"}, + {"bottom-left", "sowe"}, + {"bottom-right", "soea"}, + {"smart", "sm"}, +} + +// Gravity returns the gravity code for a name (top-left) or a code (nowe). +func Gravity(v string) (string, error) { + v = strings.ToLower(v) + for _, g := range gravityWords { + if v == g.word || v == g.code { + return g.code, nil + } + } + words := make([]string, len(gravityWords)) + for i, g := range gravityWords { + words[i] = g.word + } + return "", fmt.Errorf("must be %s", list(words)) +} + +// Focus returns the focus point gravity ("fp:X:Y") for "X:Y" or "X,Y", two +// fractions from 0 to 1 measured from the top left. +func Focus(v string) (string, error) { + sep := ":" + if strings.Contains(v, ",") { + sep = "," + } + xs, ys, ok := strings.Cut(v, sep) + x, errX := fraction(xs) + y, errY := fraction(ys) + if !ok || errX != nil || errY != nil { + return "", errors.New("must be X:Y, two numbers from 0 to 1 (0:0 is the top left)") + } + return "fp:" + x + ":" + y, nil +} + +func fraction(s string) (string, error) { + f, err := strconv.ParseFloat(s, 64) + if err != nil || f < 0 || f > 1 { + return "", errors.New("not a fraction") + } + return strconv.FormatFloat(f, 'f', -1, 64), nil +} + +// checkGravityValue accepts a gravity code with optional X:Y offsets in +// pixels, "obj" (object detection), or a focus point "fp:X:Y". The CDN turns +// an unknown code into the center. +func checkGravityValue(v string) error { + code, rest, hasRest := strings.Cut(v, ":") + if code == "fp" { + _, err := Focus(rest) + return err + } + known := code == "obj" + for _, g := range gravityWords { + known = known || code == g.code + } + if !known { + return errors.New("must be a gravity code (ce, no, so, ea, we, noea, nowe, soea, sowe, sm, obj), optionally with :X:Y offsets, or fp:X:Y") + } + if !hasRest { + return nil + } + xs, ys, ok := strings.Cut(rest, ":") + _, errX := strconv.ParseFloat(xs, 64) + _, errY := strconv.ParseFloat(ys, 64) + if !ok || errX != nil || errY != nil { + return errors.New("gravity offsets must be two numbers: " + code + ":X:Y") + } + return nil +} + +// formats are the formats the CLI can force. Each was checked against the +// CDN. AVIF is not among them: the CDN drops "f:avif" and picks the format +// from the browser's Accept header instead. +var formats = []string{"webp", "jpg", "png", "gif", "ico", "jxl", "heic"} + +// ErrAVIF explains why AVIF cannot be forced. +var ErrAVIF = errors.New("AVIF cannot be forced: the CDN drops f:avif. With auto, browsers that accept AVIF get it") + +// Format validates a format. It returns "" for auto, which means no "f:" +// option: the CDN then serves each browser AVIF or WebP when it accepts them. +func Format(v string) (string, error) { + v = strings.ToLower(v) + switch v { + case "auto": + return "", nil + case "jpeg": + return "jpg", nil + case "avif": + return "", ErrAVIF + } + for _, f := range formats { + if v == f { + return v, nil + } + } + return "", fmt.Errorf("must be %s", list(append([]string{"auto"}, formats...))) +} + +// DPR validates a pixel density from 1 to 5. +func DPR(v string) (string, error) { + n, err := strconv.Atoi(v) + if err != nil || n < 1 || n > 5 || strings.HasPrefix(v, "+") || strings.HasPrefix(v, "0") { + return "", errors.New("must be a whole number from 1 to 5") + } + return v, nil +} + +// checkCacheBuster applies the CDN's rule: it drops a cache buster unless it +// has at least 3 characters, a dot, and at least 1 more character (cache +// tokens from the dashboard look like "aB3d.1f"). +func checkCacheBuster(v string) error { + parts := strings.Split(v, ".") + if len(parts) < 2 || len(parts[0]) < 3 || len(parts[1]) < 1 { + return errors.New(`must look like "abc.1" (3 or more characters, a dot, then more); the CDN ignores any other value`) + } + return nil +} + +// list joins values as "a, b or c". +func list(values []string) string { + if len(values) < 2 { + return strings.Join(values, "") + } + return strings.Join(values[:len(values)-1], ", ") + " or " + values[len(values)-1] +} diff --git a/internal/imgurl/options_test.go b/internal/imgurl/options_test.go new file mode 100644 index 0000000..38275b4 --- /dev/null +++ b/internal/imgurl/options_test.go @@ -0,0 +1,99 @@ +package imgurl + +import ( + "errors" + "testing" +) + +func TestTypedValues(t *testing.T) { + type check func(string) (string, error) + tests := []struct { + name string + fn check + good map[string]string // input -> value written to the URL + bad []string + }{ + {"Size", Size, + map[string]string{"auto": "auto", "1": "1", "800": "800"}, + []string{"", "0", "-5", "+5", "05", "1.5", "abc", "AUTO", "99999999999"}}, + {"Quality", Quality, + map[string]string{"auto": "auto", "mauto": "mauto", "eco": "eco", "1": "1", "100": "100"}, + []string{"", "0", "101", "080", "+50", "best", "high"}}, + {"ResizeType", ResizeType, + map[string]string{"fit": "fit", "fill": "fill", "fill-down": "fill-down", "force": "force", "auto": "auto"}, + []string{"", "crop", "FILL", "cover"}}, + {"Gravity", Gravity, + map[string]string{"center": "ce", "top": "no", "bottom": "so", "left": "we", "right": "ea", + "top-left": "nowe", "top-right": "noea", "bottom-left": "sowe", "bottom-right": "soea", + "smart": "sm", "Top-Left": "nowe", "nowe": "nowe", "ce": "ce"}, + []string{"", "middle", "north", "fp", "obj"}}, + {"Focus", Focus, + map[string]string{"0.5:0.2": "fp:0.5:0.2", "0,1": "fp:0:1", "1:0.25": "fp:1:0.25", ".5:.5": "fp:0.5:0.5"}, + []string{"", "0.5", "1.5:0", "0:-0.1", "a:b", "0.5:0.5:0.5"}}, + {"Format", Format, + map[string]string{"auto": "", "webp": "webp", "JPG": "jpg", "jpeg": "jpg", "png": "png", "gif": "gif", + "ico": "ico", "jxl": "jxl", "heic": "heic"}, + []string{"", "avif", "best", "css", "bmp"}}, + {"DPR", DPR, + map[string]string{"1": "1", "2": "2", "5": "5"}, + []string{"", "0", "6", "1.5", "02", "+2"}}, + } + for _, tt := range tests { + for in, want := range tt.good { + if got, err := tt.fn(in); err != nil || got != want { + t.Errorf("%s(%q) = %q, %v; want %q", tt.name, in, got, err, want) + } + } + for _, in := range tt.bad { + if got, err := tt.fn(in); err == nil { + t.Errorf("%s(%q) = %q, want an error", tt.name, in, got) + } + } + } +} + +func TestFormatExplainsAVIF(t *testing.T) { + if _, err := Format("AVIF"); !errors.Is(err, ErrAVIF) { + t.Fatalf("Format(AVIF) error = %v, want ErrAVIF", err) + } +} + +func TestCheck(t *testing.T) { + good := [][2]string{ + {"w", "300"}, {"h", "auto"}, {"q", "eco"}, {"rt", "fill-down"}, {"dpr", "2"}, + {"g", "sm"}, {"g", "obj"}, {"g", "ce:10:-5"}, {"g", "fp:0.5:0.5"}, + {"cb", "aB3d.1f"}, {"cb", "abc.d.e"}, + {"bl", "5"}, {"c", "200:100:ce"}, {"bg", "255:255:255"}, {"f", "best"}, {"f", "avif"}, {"ig", "avif"}, + {"att", "1"}, {"no-auto", "1"}, + } + for _, g := range good { + if err := Check(g[0], g[1]); err != nil { + t.Errorf("Check(%q, %q) = %v, want nil", g[0], g[1], err) + } + } + bad := [][2]string{ + {"id", hash}, {"width", "300"}, {"W", "300"}, {"", "x"}, + {"bl", ""}, {"bl", "5/6"}, {"bl", "5 6"}, {"bl", "a?b"}, {"bl", "a#b"}, {"bl", "50%"}, {"bl", "a\x00"}, + {"w", "0"}, {"h", "x"}, {"q", "200"}, {"rt", "crop"}, {"dpr", "9"}, + {"g", "middle"}, {"g", "fp"}, {"g", "fp:2:0"}, {"g", "ce:1"}, {"g", "ce:a:b"}, + {"cb", "abc123xyz789"}, {"cb", "ab.c"}, {"cb", "abc."}, {"cb", ".abc"}, + } + for _, b := range bad { + if err := Check(b[0], b[1]); err == nil { + t.Errorf("Check(%q, %q) = nil, want an error", b[0], b[1]) + } + } +} + +func TestEveryOptionHasOnePlace(t *testing.T) { + seen := map[string]bool{} + for _, n := range cdnOptions { + if seen[n] { + t.Errorf("%q is listed twice", n) + } + seen[n] = true + } + if IsOption("id") { + t.Error(`"id" starts the source and must not be an option`) + } +} From e850cffdef00ab0a944b4c1f6af1da2171159334 Mon Sep 17 00:00:00 2001 From: selul Date: Thu, 5 Nov 2026 11:16:23 +0200 Subject: [PATCH 04/11] Add optimole url: print assets' CDN URLs, resized or converted Takes asset keys, Optimole URLs, or the JSON lines of upload --json and url --json (from stdin when there are no arguments), applies --width, --height, --resize, --gravity, --focus, --quality, --format, --dpr, --download, --set, --unset and --reset, and prints one URL per input in input order. An option already in the URL is replaced, never repeated. URLs need no API call and no key; keys are looked up in parallel. A failed input is reported and the others are still printed; a rejected key stops the run. Co-Authored-By: Claude Opus 5.5 --- README.md | 74 ++++- internal/cli/cli.go | 6 +- internal/cli/url.go | 575 +++++++++++++++++++++++++++++++++++++++ internal/cli/url_test.go | 313 +++++++++++++++++++++ 4 files changed, 962 insertions(+), 6 deletions(-) create mode 100644 internal/cli/url.go create mode 100644 internal/cli/url_test.go diff --git a/README.md b/README.md index 9aee3a6..905dd8b 100644 --- a/README.md +++ b/README.md @@ -1,12 +1,14 @@ # optimole `optimole` uploads local files into an [Optimole](https://optimole.com) media library, into the folders you -choose, and exports the library back as ZIP archives. It is a single binary with no dependencies. +choose, exports the library back as ZIP archives, and builds the CDN URLs of its images, resized or +converted. It is a single binary with no dependencies. ```sh optimole login # paste your API key once optimole upload -r ./photos --folder Campaigns # photos/2024/a.jpg -> Campaigns/2024 optimole export start # export the library, download ./optimole-export-ID +optimole url KEY --width 800 --format webp # the image's CDN URL, 800 px wide, as WebP ``` ## Install @@ -346,6 +348,67 @@ An export is `queued`, `running`, `completed`, `failed` or `cancelled`. Once eve exports (`export_limit_reached`); delete one with `optimole export delete ID`, or wait: finished exports are deleted automatically 10 days after they finish. +### `optimole url [flags] KEY|URL...` + +Prints the CDN URL of each asset key or Optimole URL, changed as the flags ask. Without arguments it reads one +key or URL per line from stdin, and also the JSON lines of `optimole upload --json` and `optimole url --json`, +so commands can be piped together. + +```sh +# An asset's URL, 800 px wide, as WebP +optimole url 42cf6621995747f69a30d32f6a6c4f82 --width 800 --format webp + +# The URLs of everything just uploaded, 1200 px wide +optimole upload -r ./photos --json | optimole url --width 1200 + +# Change a URL you already have (no API call, no key needed) +optimole url 'https://abc.i.optimole.com/w:auto/h:auto/q:auto/id:42cf…/directUpload/photo.jpg' --quality 60 + +# Chain changes +optimole url KEY --width 800 | optimole url --resize fill --height 400 --gravity top +``` + +| Flag | URL option | Meaning | +|---|---|---| +| `--width N\|auto`, `--height N\|auto` | `w:`, `h:` | Size in pixels. | +| `--resize TYPE` | `rt:` | `fit` (the default: keeps the aspect ratio and fits the size), `fill` (keeps the aspect ratio, fills the size and cuts what sticks out), `fill-down` (like `fill`, but an image smaller than the size is only cropped to its aspect ratio), `force` (ignores the aspect ratio) or `auto` (`fill` when the image and the size have the same orientation, else `fit`). | +| `--gravity POS` | `g:` | The part `fill` keeps: `center`, `top`, `bottom`, `left`, `right`, `top-left`, `top-right`, `bottom-left`, `bottom-right` or `smart`. | +| `--focus X:Y` | `g:fp:X:Y` | The point `fill` keeps, as fractions: `0:0` is the top left, `1:1` the bottom right. | +| `--quality Q` | `q:` | `auto`, `mauto`, `eco` or 1–100. | +| `--format F` | `f:` | `auto` removes `f:`, and the CDN serves each browser AVIF or WebP when it accepts them; or force `webp`, `jpg`, `png`, `gif`, `ico`, `jxl` or `heic`. | +| `--dpr N` | `dpr:` | Pixel density, 1–5. | +| `--download` | `att:1` | Serve the image as a download. | +| `--set NAME:VALUE` | any | Any other option the CDN reads, e.g. `--set bl:5` (blur) or `--set c:300:200:ce` (crop); repeatable. Unknown names are refused, because the CDN drops them silently. | +| `--unset NAME` | | Remove an option from the URL; repeatable. | +| `--reset` | | Start from `w:auto/h:auto/q:auto`, dropping the URL's options. | +| `-c`, `--concurrency N` | | Keys looked up in parallel, 1–16 (default 4). | +| `--json` | | One JSON object per input. | +| `--api-key`, `--api-url` | | See [Authentication](#authentication). | + +#### How URLs are changed + +- A key is looked up with `GET /api/assets/{key}`, which gives its URL on the account's delivery domain + (custom domains included). A URL is changed locally: no API call and no key are needed. +- An option already in the URL is replaced, never added a second time: the CDN uses only the first copy of an + option, so a second `w:` would do nothing. Options the flags do not name are kept. +- Options are written in the order the CDN applies them, so the same changes always give the same URL. +- Two flags that change the same option (`--width` and `--set w:…`, `--gravity` and `--focus`) are an error. +- Values are checked the way the CDN reads them. The CDN answers a bad `w`, `h` or `q` with an empty image, + and ignores a cache buster (`cb:`) unless it looks like `abc.1` (3 or more characters, a dot, then more), so + those are refused. +- `--format avif` is refused: the CDN drops `f:avif` and picks the format from the browser instead, which is + what `--format auto` asks for. +- Files that are not images (their URLs have `/files/`, or CSS and JS assets) are printed as they are; image + flags are an error for them. + +#### Output + +One URL per line, in input order; a failed input is reported on stderr and the others are still printed. With +`--json`, each input gives one object: `input`, `url` and, when known, `key`, `mime_type`, `original_width` +and `original_height`; a failed input gives `input`, `error` and `code` (`invalid_input`, `not_found`, +`no_url` or `not_an_image`). A key the dashboard does not know and a key outside a folder-scoped key's folders +both answer `not_found`. A rejected API key stops the run. + ### `optimole login`, `optimole logout` `login` reads the key from stdin: type or paste it at the prompt, or pipe it (`optimole login < key.txt`; @@ -362,9 +425,9 @@ Prints the version, commit and build date. | Code | Meaning | |---|---| -| 0 | Every selected file was uploaded (skipped files do not count). For `export start` and `export download`: the export completed and every selected archive is on disk (`export start --no-wait`: the export was started, or one was already running). | -| 1 | At least one file failed, there was nothing to upload, or the output could not be written. For exports: the export failed or was cancelled, an archive failed, `--timeout` passed, or the account keeps the most exports it may. | -| 2 | Usage or configuration error (bad flags, a directory without `-r`, an unknown `--domain`, missing key, non-https URL). Nothing was uploaded or downloaded. | +| 0 | Every selected file was uploaded (skipped files do not count). For `export start` and `export download`: the export completed and every selected archive is on disk (`export start --no-wait`: the export was started, or one was already running). For `url`: every input was printed. | +| 1 | At least one file failed, there was nothing to upload, or the output could not be written. For exports: the export failed or was cancelled, an archive failed, `--timeout` passed, or the account keeps the most exports it may. For `url`: an input failed (the others are still printed), or the API key was rejected. | +| 2 | Usage or configuration error (bad flags, a directory without `-r`, an unknown `--domain`, missing key, non-https URL). Nothing was uploaded or downloaded. For `url`, a missing key stops the run at the first asset key. | | 130 | Interrupted with Ctrl-C. | ## Limits @@ -373,7 +436,8 @@ Prints the version, commit and build date. Reservations count towards it until they are completed or settled by the server's hourly cleanup, which is another reason the CLI avoids reserving an asset twice for one file. - The API allows 300 requests per minute per account; each file takes 2 API requests plus the storage upload, - so very large batches may see `429` answers, which the CLI waits out. + and each key given to `optimole url` one, so very large batches may see `429` answers, which the CLI waits + out. URLs given to `optimole url` take none. - A presigned upload URL is valid for 15 minutes. - An account runs one export at a time and keeps at most 5. Export archives are about 1 GB each (a part can be larger when its first files are), and their download URLs are valid for a day. diff --git a/internal/cli/cli.go b/internal/cli/cli.go index fb7b47c..56089c7 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -62,7 +62,8 @@ func DefaultEnv(build BuildInfo) Env { } } -const mainUsage = `optimole uploads local files into an Optimole media library and exports it. +const mainUsage = `optimole uploads local files into an Optimole media library, exports it, and +builds the CDN URLs of its images. Usage: optimole [flags] [arguments] @@ -71,6 +72,7 @@ Commands: upload Upload files and directories into library folders folders List the library's folders export Export the library as ZIP archives and download them + url Print an asset's CDN URL, resized or converted login Save an API key (read from stdin) logout Delete the saved API key version Print the version @@ -92,6 +94,8 @@ func Run(ctx context.Context, args []string, env Env) int { return runFolders(ctx, rest, env) case "export": return runExport(ctx, rest, env) + case "url": + return runURL(ctx, rest, env) case "login": return runLogin(ctx, rest, env) case "logout": diff --git a/internal/cli/url.go b/internal/cli/url.go new file mode 100644 index 0000000..be4068f --- /dev/null +++ b/internal/cli/url.go @@ -0,0 +1,575 @@ +package cli + +import ( + "bufio" + "context" + "encoding/json" + "errors" + "flag" + "fmt" + "io" + "net/http" + "os" + "regexp" + "strings" + "sync" + + "github.com/Codeinwp/optimole-cli/internal/api" + "github.com/Codeinwp/optimole-cli/internal/imgurl" + "github.com/Codeinwp/optimole-cli/internal/output" + "github.com/Codeinwp/optimole-cli/internal/termecho" +) + +const urlUsage = `Usage: + optimole url [flags] KEY|URL... + COMMAND | optimole url [flags] + +Prints the CDN URL of each asset key or Optimole URL, changed as the flags +ask. Without arguments it reads one key or URL per line from stdin, and also +the JSON lines of "optimole upload --json" and "optimole url --json", so +commands can be piped together: + + optimole url 42cf6621995747f69a30d32f6a6c4f82 --width 800 --format webp + optimole upload -r ./photos --json | optimole url --width 1200 + optimole url KEY --width 800 | optimole url --quality 60 + +URLs are changed locally, with no API call. Keys are looked up with the API, +which needs an API key. An option already in the URL is replaced, because the +CDN uses only the first copy of an option; options the flags do not name are +kept. + +Image flags: + --width N|auto Width in pixels + --height N|auto Height in pixels + --resize TYPE fit (the default: keeps the aspect ratio and fits the size), + fill (fills the size and cuts what sticks out), fill-down, + force (ignores the aspect ratio) or auto + --gravity POS The part fill keeps: center, top, bottom, left, right, + top-left, top-right, bottom-left, bottom-right or smart + --focus X:Y The point fill keeps: 0:0 is the top left, 1:1 the bottom right + --quality Q auto, mauto, eco or 1-100 + --format F auto (each browser gets AVIF or WebP when it accepts them), + webp, jpg, png, gif, ico, jxl or heic + --dpr N Pixel density, 1-5 + --download Serve the image as a download + --set NAME:VALUE Set another CDN option, e.g. --set bl:5 for a blur (repeatable) + --unset NAME Remove an option from the URL (repeatable) + --reset Start from w:auto/h:auto/q:auto, dropping the URL's options + +Other flags: + -c, --concurrency N Keys looked up in parallel, 1-16 (default 4) + --json Print one JSON object per input + +Files that are not images (their URLs have /files/) are printed as they are; +image flags are an error for them. + +Exit codes: 0 every input printed, 1 an input failed (the others are still +printed), 2 usage or configuration error, 130 interrupted. +` + commonUsage + +const ( + urlDefaultConcurrency = 4 + urlMaxConcurrency = 16 + // maxURLLine caps a line read from stdin. + maxURLLine = 1 << 20 +) + +var keyPattern = regexp.MustCompile(`^[0-9a-fA-F]{32}$`) + +// urlEdits is what the flags change in every URL. +type urlEdits struct { + reset bool + set []imgurl.Option + unset []string +} + +func (e urlEdits) empty() bool { return !e.reset && len(e.set) == 0 && len(e.unset) == 0 } + +func (e urlEdits) apply(u *imgurl.URL) { + if e.reset { + u.Reset() + } + for _, o := range e.set { + u.Set(o.Name, o.Value) + } + for _, n := range e.unset { + u.Unset(n) + } +} + +// urlFlags are the flags of "optimole url". +type urlFlags struct { + width, height, resize, gravity, focus, quality, format, dpr string + download, reset bool + set, unset stringList +} + +// typedOption maps an image flag to the option it sets. +type typedOption struct { + flag string + option string + value *string + check func(string) (string, error) +} + +// edits turns the flags given (visited) into the changes to apply. Two flags +// that change the same option are a usage error. +func (f *urlFlags) edits(visited map[string]bool) (urlEdits, error) { + e := urlEdits{reset: f.reset} + owner := map[string]string{} // option -> flag that changes it + claim := func(option, flag string) error { + if prev, ok := owner[option]; ok { + return fmt.Errorf("%s and %s both change the %s option", prev, flag, option) + } + owner[option] = flag + return nil + } + typed := []typedOption{ + {"--width", "w", &f.width, imgurl.Size}, + {"--height", "h", &f.height, imgurl.Size}, + {"--resize", "rt", &f.resize, imgurl.ResizeType}, + {"--gravity", "g", &f.gravity, imgurl.Gravity}, + {"--focus", "g", &f.focus, imgurl.Focus}, + {"--quality", "q", &f.quality, imgurl.Quality}, + {"--format", "f", &f.format, imgurl.Format}, + {"--dpr", "dpr", &f.dpr, imgurl.DPR}, + } + for _, t := range typed { + if !visited[strings.TrimPrefix(t.flag, "--")] { + continue + } + if err := claim(t.option, t.flag); err != nil { + return e, err + } + v, err := t.check(*t.value) + if err != nil { + return e, fmt.Errorf("%s %s: %v", t.flag, *t.value, err) + } + if v == "" { // --format auto + e.unset = append(e.unset, t.option) + continue + } + e.set = append(e.set, imgurl.Option{Name: t.option, Value: v}) + } + if f.download { + if err := claim("att", "--download"); err != nil { + return e, err + } + e.set = append(e.set, imgurl.Option{Name: "att", Value: "1"}) + } + for _, s := range f.set { + name, value, ok := strings.Cut(s, ":") + if !ok { + return e, fmt.Errorf("--set %s: must be NAME:VALUE, e.g. bl:5", s) + } + if err := imgurl.Check(name, value); err != nil { + return e, fmt.Errorf("--set %s: %v", s, err) + } + if err := claim(name, "--set "+s); err != nil { + return e, err + } + e.set = append(e.set, imgurl.Option{Name: name, Value: value}) + } + for _, name := range f.unset { + if !imgurl.IsOption(name) { + return e, fmt.Errorf("--unset %s: %q is not an option the CDN reads", name, name) + } + if err := claim(name, "--unset "+name); err != nil { + return e, err + } + e.unset = append(e.unset, name) + } + return e, nil +} + +func runURL(ctx context.Context, args []string, env Env) int { + fs := newFlagSet("url", urlUsage, env) + var common commonFlags + common.register(fs, true) + var f urlFlags + var jsonOut bool + concurrency := urlDefaultConcurrency + for _, s := range []struct { + name string + v *string + }{ + {"width", &f.width}, {"height", &f.height}, {"resize", &f.resize}, {"gravity", &f.gravity}, + {"focus", &f.focus}, {"quality", &f.quality}, {"format", &f.format}, {"dpr", &f.dpr}, + } { + fs.StringVar(s.v, s.name, "", "") + } + fs.BoolVar(&f.download, "download", false, "") + fs.BoolVar(&f.reset, "reset", false, "") + fs.Var(&f.set, "set", "") + fs.Var(&f.unset, "unset", "") + fs.IntVar(&concurrency, "c", urlDefaultConcurrency, "") + fs.IntVar(&concurrency, "concurrency", urlDefaultConcurrency, "") + fs.BoolVar(&jsonOut, "json", false, "") + rest, err := parseArgs(fs, args) + if err != nil { + return flagExit(err) + } + visited := map[string]bool{} + fs.Visit(func(fl *flag.Flag) { visited[fl.Name] = true }) + edits, err := f.edits(visited) + if err != nil { + return usageError(env, "url", "%v", err) + } + if concurrency < 1 || concurrency > urlMaxConcurrency { + return usageError(env, "url", "--concurrency must be between 1 and %d", urlMaxConcurrency) + } + + inputs := func(context.Context) (<-chan string, func() error) { + ch := make(chan string, len(rest)) + for _, a := range rest { + ch <- a + } + close(ch) + return ch, func() error { return nil } + } + if len(rest) == 0 || (len(rest) == 1 && rest[0] == "-") { + if file, ok := env.Stdin.(*os.File); ok && termecho.IsTerminal(file) { + return usageError(env, "url", "give asset keys or URLs, or pipe them in") + } + inputs = func(ctx context.Context) (<-chan string, func() error) { return readLines(ctx, env.Stdin) } + } + + // The first key connects inside a lookup, whose warnings would otherwise + // race the results written to stderr. + env.Stderr = &lockedWriter{w: env.Stderr} + run := &urlRun{env: env, common: &common, edits: edits} + return run.run(ctx, inputs, concurrency, jsonOut) +} + +// lockedWriter serializes writes from several goroutines. +type lockedWriter struct { + mu sync.Mutex + w io.Writer +} + +func (l *lockedWriter) Write(p []byte) (int, error) { + l.mu.Lock() + defer l.mu.Unlock() + return l.w.Write(p) +} + +// readLines streams r line by line. The returned function gives the read +// error, if any, once the channel is closed. +func readLines(ctx context.Context, r io.Reader) (<-chan string, func() error) { + ch := make(chan string) + var err error + go func() { + defer close(ch) + sc := bufio.NewScanner(r) + sc.Buffer(make([]byte, 0, 64*1024), maxURLLine) + for sc.Scan() { + select { + case ch <- sc.Text(): + case <-ctx.Done(): + return + } + } + if e := sc.Err(); e != nil { + if errors.Is(e, bufio.ErrTooLong) { + e = fmt.Errorf("a line is longer than %d bytes", maxURLLine) + } + err = fmt.Errorf("reading stdin: %w", e) + } + }() + // err is written before close(ch), and read only after it. + return ch, func() error { return err } +} + +// urlInput is one input line, sorted out. +type urlInput struct { + shown string // what messages call it + key string // an asset key to look up + url string // a URL to change + // Details a JSON line carried along. + mimeType string + width, height int64 + err error // neither a key nor a URL + skip bool // blank, or an upload summary line +} + +func classify(line string) urlInput { + s := strings.TrimSpace(line) + switch { + case s == "": + return urlInput{skip: true} + case strings.HasPrefix(s, "{"): + return classifyJSON(s) + case keyPattern.MatchString(s): + return urlInput{shown: s, key: strings.ToLower(s)} + case hasHTTPScheme(s): + return urlInput{shown: s, url: s} + } + return urlInput{shown: shorten(s), err: errors.New("not an asset key (32 hexadecimal characters) or an Optimole URL")} +} + +// classifyJSON reads a line of "optimole upload --json" or "optimole url --json". +func classifyJSON(s string) urlInput { + var line struct { + URL string `json:"url"` + Path string `json:"path"` + Status string `json:"status"` + MimeType string `json:"mime_type"` + OriginalWidth int64 `json:"original_width"` + OriginalHeight int64 `json:"original_height"` + Summary json.RawMessage `json:"summary"` + } + if err := json.Unmarshal([]byte(s), &line); err != nil { + return urlInput{shown: shorten(s), err: errors.New("not valid JSON")} + } + if line.Summary != nil { + return urlInput{skip: true} + } + if line.URL != "" { + return urlInput{shown: line.URL, url: line.URL, mimeType: line.MimeType, width: line.OriginalWidth, height: line.OriginalHeight} + } + in := urlInput{shown: shorten(s)} + if line.Path != "" { + in.shown = line.Path + } + msg := `the JSON line has no "url"` + if line.Status != "" { + msg += " (status " + line.Status + ")" + } + in.err = errors.New(msg) + return in +} + +func hasHTTPScheme(s string) bool { + l := strings.ToLower(s) + return strings.HasPrefix(l, "https://") || strings.HasPrefix(l, "http://") +} + +// shorten keeps messages about a long line readable. +func shorten(s string) string { + const limit = 120 + if r := []rune(s); len(r) > limit { + return string(r[:limit]) + "…" + } + return s +} + +// urlResult is the outcome of one input. +type urlResult struct { + in urlInput + url string + key string + err error + code string + // fatal, when not zero, stops the run with this exit code after + // printing fatalMsg: no later input can succeed. + fatal int + fatalMsg string +} + +// urlRun holds what every input shares. +type urlRun struct { + env Env + common *commonFlags + edits urlEdits + + once sync.Once + sess *session + sessErr error +} + +// session connects on the first key, so URL-only runs need no API key. +func (r *urlRun) session() (*session, error) { + r.once.Do(func() { r.sess, r.sessErr = r.common.connect(r.env) }) + return r.sess, r.sessErr +} + +// run resolves the inputs, up to concurrency at a time, and prints the +// results in input order. +func (r *urlRun) run(ctx context.Context, inputs func(context.Context) (<-chan string, func() error), concurrency int, jsonOut bool) int { + runCtx, cancel := context.WithCancel(ctx) + defer cancel() + lines, readErr := inputs(runCtx) + results := make(chan chan urlResult, concurrency) + go func() { + defer close(results) + sem := make(chan struct{}, concurrency) + for line := range lines { + in := classify(line) + if in.skip { + continue + } + ch := make(chan urlResult, 1) + select { + case sem <- struct{}{}: + case <-runCtx.Done(): + return + } + select { + case results <- ch: + case <-runCtx.Done(): + return + } + go func() { + defer func() { <-sem }() + ch <- r.resolve(runCtx, in) + }() + } + }() + + out := output.NewErrWriter(r.env.Stdout) + enc := json.NewEncoder(out) + enc.SetEscapeHTML(false) + interrupted := func() int { + fmt.Fprintln(r.env.Stderr, "optimole url: interrupted") + return ExitInterrupted + } + failed := 0 + for ch := range results { + var res urlResult + select { + case res = <-ch: + case <-ctx.Done(): + return interrupted() + } + if ctx.Err() != nil { + return interrupted() + } + if res.fatal != 0 { + cancel() + fmt.Fprintln(r.env.Stderr, res.fatalMsg) + return res.fatal + } + if res.err != nil { + failed++ + if jsonOut { + enc.Encode(urlLine{Input: res.in.shown, Error: res.err.Error(), Code: res.code}) + } else { + fmt.Fprintf(r.env.Stderr, "optimole url: %s: %v\n", res.in.shown, res.err) + } + } else if jsonOut { + enc.Encode(urlLine{ + Input: res.in.shown, URL: res.url, Key: res.key, MimeType: res.in.mimeType, + OriginalWidth: res.in.width, OriginalHeight: res.in.height, + }) + } else { + fmt.Fprintln(out, res.url) + } + if err := out.Err(); err != nil { + return outputFailed(r.env, err) + } + } + if ctx.Err() != nil { + return interrupted() + } + if err := readErr(); err != nil { + fmt.Fprintf(r.env.Stderr, "optimole url: %v\n", err) + return ExitFailure + } + if failed > 0 { + return ExitFailure + } + return ExitOK +} + +// urlLine is the JSON object printed for each input. +type urlLine struct { + Input string `json:"input"` + URL string `json:"url,omitempty"` + Key string `json:"key,omitempty"` + MimeType string `json:"mime_type,omitempty"` + OriginalWidth int64 `json:"original_width,omitempty"` + OriginalHeight int64 `json:"original_height,omitempty"` + Error string `json:"error,omitempty"` + Code string `json:"code,omitempty"` +} + +// Codes of the errors "optimole url" reports per input. +const ( + codeInvalidInput = "invalid_input" + codeNotFound = "not_found" + codeNoURL = "no_url" + codeNotAnImage = "not_an_image" +) + +// resolve turns one input into its URL. +func (r *urlRun) resolve(ctx context.Context, in urlInput) urlResult { + res := urlResult{in: in} + if in.err != nil { + res.err, res.code = in.err, codeInvalidInput + return res + } + raw := in.url + if in.key != "" { + var ok bool + if raw, ok = r.lookUp(ctx, &res); !ok { + return res + } + } + u, err := imgurl.Parse(raw) + if err != nil { + res.err, res.code = err, codeInvalidInput + return res + } + if !r.edits.empty() && u.Kind() == imgurl.File { + res.err, res.code = errors.New("not an image (a file URL): only image URLs can be changed"), codeNotAnImage + return res + } + r.edits.apply(u) + res.url = u.String() + if res.key == "" { + res.key = in.key + } + if res.key == "" && keyPattern.MatchString(u.ID()) { + res.key = strings.ToLower(u.ID()) + } + return res +} + +// lookUp fetches the URL of res.in.key. It fills res and returns false when +// the input failed or the run must stop. +func (r *urlRun) lookUp(ctx context.Context, res *urlResult) (string, bool) { + key := res.in.key + sess, err := r.session() + if err != nil { + res.fatal, res.fatalMsg = ExitUsage, fmt.Sprintf("optimole url: %v", err) + return "", false + } + a, err := sess.client.GetAsset(ctx, key) + var apiErr *api.Error + switch { + case err == nil: + case errors.Is(err, context.Canceled): + // Ctrl-C, or the run stopping after a fatal error: either way the + // result is never printed. + res.fatal, res.fatalMsg = ExitInterrupted, "optimole url: interrupted" + return "", false + case isRejectedKey(err): + res.fatal = ExitFailure + res.fatalMsg = fmt.Sprintf("optimole url: %s\nThe API key (from %s) was rejected.", sess.redact.Error(err), sess.settings.KeySource) + return "", false + case errors.As(err, &apiErr) && (apiErr.StatusCode == http.StatusForbidden || apiErr.StatusCode == http.StatusNotFound): + res.err, res.code = errors.New("no such asset, or this API key cannot see it"), codeNotFound + return "", false + default: + res.err = errors.New(sess.redact.Error(err)) + if errors.As(err, &apiErr) { + res.code = apiErr.Code + } + return "", false + } + res.key = key + if res.in.mimeType == "" { + res.in.mimeType = a.MimeType + } + if res.in.width == 0 && a.Width.Valid { + res.in.width = a.Width.Value + } + if res.in.height == 0 && a.Height.Valid { + res.in.height = a.Height.Value + } + if a.URL == "" { + res.err, res.code = errors.New("the asset has no CDN URL yet"), codeNoURL + return "", false + } + return a.URL, true +} diff --git a/internal/cli/url_test.go b/internal/cli/url_test.go new file mode 100644 index 0000000..4583896 --- /dev/null +++ b/internal/cli/url_test.go @@ -0,0 +1,313 @@ +package cli + +import ( + "context" + "net/http" + "strings" + "sync" + "testing" + "time" + + "github.com/Codeinwp/optimole-cli/internal/config" + "github.com/Codeinwp/optimole-cli/internal/testserver" +) + +const ( + urlHash = "42cf6621995747f69a30d32f6a6c4f82" + urlImage = "https://mlabc.i.optimole.com/w:auto/h:auto/q:auto/id:" + urlHash + "/directUpload/photo.jpg" +) + +// libURL is the URL the fake dashboard gives an image asset. +func libURL(key, name string) string { + return "https://app.i.optimole.com/w:auto/h:auto/q:auto/id:" + key + "/directUpload/" + name +} + +func TestURLChangesURLsWithoutTheAPI(t *testing.T) { + h := newHarness(t) // no API key: URLs need none + tests := []struct { + args []string + want string + }{ + {[]string{urlImage, "--width", "800", "--format", "webp"}, + "https://mlabc.i.optimole.com/w:800/h:auto/q:auto/f:webp/id:" + urlHash + "/directUpload/photo.jpg"}, + {[]string{"--resize", "fill", "--width", "400", "--height", "300", "--gravity", "top-left", urlImage}, + "https://mlabc.i.optimole.com/w:400/h:300/q:auto/rt:fill/g:nowe/id:" + urlHash + "/directUpload/photo.jpg"}, + {[]string{urlImage, "--focus", "0.25:0.75", "--quality", "60", "--dpr", "2"}, + "https://mlabc.i.optimole.com/w:auto/h:auto/q:60/g:fp:0.25:0.75/dpr:2/id:" + urlHash + "/directUpload/photo.jpg"}, + {[]string{urlImage, "--download", "--set", "bl:5", "--set", "cb:aB3d.1f"}, + "https://mlabc.i.optimole.com/cb:aB3d.1f/w:auto/h:auto/q:auto/bl:5/att:1/id:" + urlHash + "/directUpload/photo.jpg"}, + // An option already there is replaced, never repeated. + {[]string{"https://mlabc.i.optimole.com/w:300/h:200/q:60/f:webp/bl:5/id:" + urlHash + "/directUpload/a.jpg", "--width", "100"}, + "https://mlabc.i.optimole.com/w:100/h:200/q:60/f:webp/bl:5/id:" + urlHash + "/directUpload/a.jpg"}, + {[]string{"https://mlabc.i.optimole.com/w:300/h:200/q:60/f:webp/bl:5/id:" + urlHash + "/directUpload/a.jpg", "--format", "auto", "--unset", "bl"}, + "https://mlabc.i.optimole.com/w:300/h:200/q:60/id:" + urlHash + "/directUpload/a.jpg"}, + {[]string{"https://mlabc.i.optimole.com/w:300/h:200/q:60/f:webp/bl:5/id:" + urlHash + "/directUpload/a.jpg", "--reset", "--width", "50"}, + "https://mlabc.i.optimole.com/w:50/h:auto/q:auto/id:" + urlHash + "/directUpload/a.jpg"}, + // No flags: the URL as the CDN reads it. + {[]string{urlImage}, urlImage}, + } + for _, tt := range tests { + if code := h.run(append([]string{"url"}, tt.args...)...); code != ExitOK { + t.Fatalf("url %v: exit %d, stderr %s", tt.args, code, h.stderr.String()) + } + if got := strings.TrimSpace(h.stdout.String()); got != tt.want { + t.Errorf("url %v:\n got %s\nwant %s", tt.args, got, tt.want) + } + } + if n := len(h.srv.Events()); n != 0 { + t.Fatalf("%d API calls for URL inputs", n) + } +} + +func TestURLLooksUpKeys(t *testing.T) { + h := newHarness(t).withKey() + key := h.srv.AddAsset(testserver.Asset{Filename: "photo.jpg", ContentType: "image/jpeg", Size: 5, Width: 1200, Height: 800}) + if code := h.run("url", strings.ToUpper(key), "--width", "300"); code != ExitOK { + t.Fatalf("exit %d: %s", code, h.stderr.String()) + } + want := "https://app.i.optimole.com/w:300/h:auto/q:auto/id:" + key + "/directUpload/photo.jpg" + if got := strings.TrimSpace(h.stdout.String()); got != want { + t.Fatalf("got %s, want %s", got, want) + } + if code := h.run("url", key, "--json"); code != ExitOK { + t.Fatalf("exit %d: %s", code, h.stderr.String()) + } + lines := h.jsonLines() + if len(lines) != 1 { + t.Fatalf("lines = %v", lines) + } + got := lines[0] + if got["input"] != key || got["url"] != libURL(key, "photo.jpg") || got["key"] != key || + got["mime_type"] != "image/jpeg" || got["original_width"] != 1200.0 || got["original_height"] != 800.0 { + t.Fatalf("line = %v", got) + } +} + +func TestURLReadsStdinInOrderWhileLookingUpInParallel(t *testing.T) { + h := newHarness(t).withKey() + slow := h.srv.AddAsset(testserver.Asset{Filename: "slow.jpg", ContentType: "image/jpeg"}) + fast := h.srv.AddAsset(testserver.Asset{Filename: "fast.jpg", ContentType: "image/jpeg"}) + // The first lookup answers only once the second one has arrived: the + // lookups run together, and the output still follows the input. + arrived := make(chan struct{}) + var once sync.Once + h.srv.Hook = func(route testserver.Route, r *http.Request) { + if route != testserver.RouteAsset { + return + } + if strings.HasSuffix(r.URL.Path, slow) { + select { + case <-arrived: + case <-time.After(5 * time.Second): + t.Error("the lookups did not run in parallel") + } + return + } + once.Do(func() { close(arrived) }) + } + h.stdin = strings.NewReader(strings.Join([]string{ + slow, + "", + urlImage, + `{"path":"a.jpg","folder":"","status":"uploaded","key":"` + urlHash + `","url":"` + urlImage + `"}`, + `{"path":"b.psd","folder":"","status":"skipped","error":"type not allowed"}`, + fast, + "nonsense", + `{"summary":{"total":2,"uploaded":1,"skipped":1,"failed":0,"ignored":0}}`, + }, "\n")) + code := h.run("url", "--width", "64") + if code != ExitFailure { + t.Fatalf("exit %d, want 1 (two bad lines): %s", code, h.stderr.String()) + } + w64 := func(u string) string { return strings.Replace(u, "/w:auto/", "/w:64/", 1) } + want := strings.Join([]string{w64(libURL(slow, "slow.jpg")), w64(urlImage), w64(urlImage), w64(libURL(fast, "fast.jpg"))}, "\n") + if got := strings.TrimSpace(h.stdout.String()); got != want { + t.Fatalf("stdout:\n%s\nwant:\n%s", got, want) + } + stderr := h.stderr.String() + for _, msg := range []string{ + `optimole url: b.psd: the JSON line has no "url" (status skipped)`, + "optimole url: nonsense: not an asset key", + } { + if !strings.Contains(stderr, msg) { + t.Errorf("stderr lacks %q:\n%s", msg, stderr) + } + } +} + +func TestURLReadsUploadOutput(t *testing.T) { + h := newHarness(t).withKey() + root := tree(t, map[string]string{"a.jpg": "aaa", "b.png": "bbb"}) + if code := h.run("upload", "-r", root, "--json"); code != ExitOK { + t.Fatalf("upload: exit %d: %s", code, h.stderr.String()) + } + // The upload's summary line is skipped, not reported as a bad line. + h.stdin = strings.NewReader(h.stdout.String()) + if code := h.run("url", "--width", "100"); code != ExitOK || h.stderr.Len() != 0 { + t.Fatalf("url: exit %d, stderr %q", code, h.stderr.String()) + } + got := strings.Split(strings.TrimSpace(h.stdout.String()), "\n") + if len(got) != 2 || !strings.Contains(got[0]+got[1], "/w:100/h:auto/q:auto/id:") { + t.Fatalf("stdout = %q", got) + } + if n := h.srv.Count(testserver.RouteAsset); n != 0 { + t.Fatalf("%d lookups: the upload lines carry their URLs", n) + } +} + +func TestURLPipesIntoItself(t *testing.T) { + h := newHarness(t).withKey() + key := h.srv.AddAsset(testserver.Asset{Filename: "photo.jpg", ContentType: "image/jpeg", Width: 640, Height: 480}) + if code := h.run("url", key, "--width", "320", "--json"); code != ExitOK { + t.Fatalf("exit %d: %s", code, h.stderr.String()) + } + h.stdin = strings.NewReader(h.stdout.String()) + if code := h.run("url", "--format", "webp", "--json"); code != ExitOK { + t.Fatalf("exit %d: %s", code, h.stderr.String()) + } + got := h.jsonLines()[0] + want := "https://app.i.optimole.com/w:320/h:auto/q:auto/f:webp/id:" + key + "/directUpload/photo.jpg" + // The asset's details travel along the pipe. + if got["url"] != want || got["key"] != key || got["mime_type"] != "image/jpeg" || got["original_width"] != 640.0 { + t.Fatalf("line = %v", got) + } + if n := h.srv.Count(testserver.RouteAsset); n != 1 { + t.Fatalf("%d lookups, want 1: the second command reads the URL", n) + } +} + +func TestURLFiles(t *testing.T) { + h := newHarness(t).withKey() + key := h.srv.AddAsset(testserver.Asset{Filename: "notes.pdf", ContentType: "application/pdf"}) + fileURL := "https://app.i.optimole.com/files/id:" + key + "/directUpload/notes.pdf" + if code := h.run("url", key); code != ExitOK || strings.TrimSpace(h.stdout.String()) != fileURL { + t.Fatalf("exit %d, stdout %q, stderr %s", code, h.stdout.String(), h.stderr.String()) + } + if code := h.run("url", key, "--width", "100", "--json"); code != ExitFailure { + t.Fatalf("exit %d, want 1", code) + } + if got := h.jsonLines()[0]; got["code"] != "not_an_image" || got["url"] != nil { + t.Fatalf("line = %v", got) + } +} + +func TestURLLookupFailures(t *testing.T) { + h := newHarness(t).withKey() + good := h.srv.AddAsset(testserver.Asset{Filename: "a.jpg", ContentType: "image/jpeg"}) + pending := h.srv.AddAsset(testserver.Asset{Filename: "big.zip", ContentType: "application/zip", NoURL: true}) + unknown := strings.Repeat("ab", 16) + code := h.run("url", unknown, pending, good, "--json") + if code != ExitFailure { + t.Fatalf("exit %d, want 1", code) + } + lines := h.jsonLines() + if len(lines) != 3 || lines[0]["code"] != "not_found" || lines[1]["code"] != "no_url" || lines[2]["url"] != libURL(good, "a.jpg") { + t.Fatalf("lines = %v", lines) + } + if !strings.Contains(lines[0]["error"].(string), "no such asset, or this API key cannot see it") { + t.Fatalf("error = %v", lines[0]["error"]) + } +} + +func TestURLStopsOnARejectedKey(t *testing.T) { + h := newHarness(t) + h.vars[config.EnvAPIKey] = "optml-not-a-key" + code := h.run("url", urlImage, strings.Repeat("ab", 16), strings.Repeat("cd", 16), strings.Repeat("ef", 16)) + if code != ExitFailure { + t.Fatalf("exit %d, want 1", code) + } + if got := strings.TrimSpace(h.stdout.String()); got != urlImage { + t.Fatalf("stdout %q: the URL before the keys is printed, nothing after", got) + } + if n := strings.Count(h.stderr.String(), "was rejected"); n != 1 { + t.Fatalf("the rejection is reported %d times:\n%s", n, h.stderr.String()) + } + if strings.Contains(h.stderr.String(), "optml-not-a-key") { + t.Fatal("the key is in the output") + } +} + +func TestURLNeedsAnAPIKeyOnlyForKeys(t *testing.T) { + h := newHarness(t) + code := h.run("url", urlImage, urlHash) + if code != ExitUsage { + t.Fatalf("exit %d, want 2", code) + } + if got := strings.TrimSpace(h.stdout.String()); got != urlImage { + t.Fatalf("stdout %q", got) + } + if !strings.Contains(h.stderr.String(), "API key") { + t.Fatalf("stderr: %s", h.stderr.String()) + } +} + +func TestURLUsageErrors(t *testing.T) { + h := newHarness(t).withKey() + for _, args := range [][]string{ + {"--format", "avif"}, + {"--format", "best"}, + {"--set", "cb:abc123"}, + {"--width", "10", "--set", "w:20"}, + {"--gravity", "top", "--focus", "0.5:0.5"}, + {"--format", "webp", "--unset", "f"}, + {"--unset", "id"}, + {"--unset", "width"}, + {"--set", "id:" + urlHash}, + {"--set", "nocolon"}, + {"--set", "bl:1/2"}, + {"--width", "0"}, + {"--width", ""}, + {"--resize", "crop"}, + {"--quality", "101"}, + {"--focus", "2:2"}, + {"-c", "0"}, + {"-c", "17"}, + } { + code := h.run(append([]string{"url", urlImage}, args...)...) + if code != ExitUsage || h.stdout.Len() != 0 { + t.Errorf("url %v: exit %d, stdout %q; want 2 and nothing printed", args, code, h.stdout.String()) + } + } + if n := len(h.srv.Events()); n != 0 { + t.Fatalf("%d API calls for usage errors", n) + } +} + +func TestURLInterrupted(t *testing.T) { + h := newHarness(t).withKey() + key := h.srv.AddAsset(testserver.Asset{Filename: "a.jpg", ContentType: "image/jpeg"}) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + h.srv.Hook = func(route testserver.Route, _ *http.Request) { + if route == testserver.RouteAsset { + cancel() + } + } + if code := h.runCtx(ctx, "url", key); code != ExitInterrupted { + t.Fatalf("exit %d, want 130", code) + } + if !strings.Contains(h.stderr.String(), "interrupted") || h.stdout.Len() != 0 { + t.Fatalf("stdout %q, stderr %q", h.stdout.String(), h.stderr.String()) + } +} + +func TestURLRefusesAnOverlongLine(t *testing.T) { + h := newHarness(t) + h.stdin = strings.NewReader(urlImage + "\n" + strings.Repeat("x", maxURLLine+1) + "\n") + if code := h.run("url"); code != ExitFailure { + t.Fatalf("exit %d, want 1", code) + } + if strings.TrimSpace(h.stdout.String()) != urlImage || !strings.Contains(h.stderr.String(), "longer than") { + t.Fatalf("stdout %q, stderr %q", h.stdout.String(), h.stderr.String()) + } +} + +func TestURLHelp(t *testing.T) { + h := newHarness(t) + if code := h.run("help"); code != ExitOK || !strings.Contains(h.stdout.String(), " url ") { + t.Fatalf("main help does not list url: %s", h.stdout.String()) + } + if code := h.run("url", "-h"); code != ExitOK || !strings.Contains(h.stderr.String(), "--format F") { + t.Fatalf("exit %d, help: %s", code, h.stderr.String()) + } +} From cce184bbb4660e0402062e2d16153da2b7e9a185 Mon Sep 17 00:00:00 2001 From: selul Date: Thu, 5 Nov 2026 11:44:02 +0200 Subject: [PATCH 05/11] url: fix the review findings - Ctrl-C now stops a run waiting on an idle stdin pipe. - A URL with spaces or control characters is refused, so a newline cannot turn one printed URL into two; messages quote control characters. - Messages hide the API keys in use, and a line that looks like an API key is not echoed. - A URL whose origin path holds a segment the CDN reads as an option (w:9 with no w: of its own) is refused: the CDN would misread it. - Raw values match what the CDN reads: fp:X:Y with colons, cache busters without a colon, finite coordinates. - A custom-domain URL whose path starts with the origin parses, so a URL left without options can be piped back. - api.IsAssetKey replaces a copy of the key pattern; dead code and narrating comments removed. Tests cover the lookup limit, an idle pipe, output failures, newline injection and key redaction, and the fuzz test edits the URLs it reparses. Co-Authored-By: Claude Opus 5.5 --- README.md | 6 +- internal/api/client.go | 4 + internal/cli/cli.go | 36 ++++++-- internal/cli/url.go | 59 +++++++++---- internal/cli/url_test.go | 135 ++++++++++++++++++++++++++++++ internal/imgurl/imgurl.go | 39 +++++++-- internal/imgurl/imgurl_test.go | 51 +++++++++++ internal/imgurl/options.go | 33 +++++--- internal/imgurl/options_test.go | 5 +- internal/testserver/testserver.go | 7 +- 10 files changed, 328 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index 905dd8b..094ab43 100644 --- a/README.md +++ b/README.md @@ -400,6 +400,9 @@ optimole url KEY --width 800 | optimole url --resize fill --height 400 --gravity what `--format auto` asks for. - Files that are not images (their URLs have `/files/`, or CSS and JS assets) are printed as they are; image flags are an error for them. +- A URL the CDN would misread is refused: one with spaces or control characters (a newline would also print + two URLs), or one whose origin path holds a segment the CDN takes for an option, such as `w:9` when the URL + sets no `w:` of its own. #### Output @@ -407,7 +410,8 @@ One URL per line, in input order; a failed input is reported on stderr and the o `--json`, each input gives one object: `input`, `url` and, when known, `key`, `mime_type`, `original_width` and `original_height`; a failed input gives `input`, `error` and `code` (`invalid_input`, `not_found`, `no_url` or `not_an_image`). A key the dashboard does not know and a key outside a folder-scoped key's folders -both answer `not_found`. A rejected API key stops the run. +both answer `not_found`. A rejected API key stops the run. Messages never show the API keys in use (flag, +environment or saved key), and a line that looks like an API key is not echoed at all. ### `optimole login`, `optimole logout` diff --git a/internal/api/client.go b/internal/api/client.go index a496aac..cb78a87 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -48,6 +48,10 @@ const ( var assetKeyPattern = regexp.MustCompile(`^[0-9a-fA-F]{32}$`) +// IsAssetKey reports whether s has the form of an asset key: 32 hexadecimal +// characters. +func IsAssetKey(s string) bool { return assetKeyPattern.MatchString(s) } + // Options configures a Client. type Options struct { // BaseURL is the API root, e.g. https://dashboard.optimole.com/api. diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 56089c7..0998848 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -190,9 +190,20 @@ func configPath(env Env) (string, error) { return config.Path(dir), nil } -// connect resolves the key and URL and builds a client. Errors are -// configuration errors (exit 2); they never contain the key. -func (c *commonFlags) connect(env Env) (*session, error) { +// resolved is what the connection flags, the environment and the saved +// config lead to. +type resolved struct { + settings config.Settings + err error + path string + pathErr error + // redact hides every key involved: the one used, and any given by flag + // or environment. + redact redact.Redactor +} + +// resolve finds the key and URL without connecting. +func (c *commonFlags) resolve(env Env) resolved { path, pathErr := configPath(env) in := config.Inputs{FlagAPIKey: c.apiKey, FlagAPIURL: c.apiURL, Getenv: env.Getenv} if pathErr == nil { @@ -203,15 +214,22 @@ func (c *commonFlags) connect(env Env) (*session, error) { if env.Getenv != nil { envKey = env.Getenv(config.EnvAPIKey) } - red := redact.New(settings.APIKey, c.apiKey, envKey) - if err != nil { - if errors.Is(err, config.ErrNoAPIKey) && pathErr != nil { - return nil, fmt.Errorf("%w (%v)", err, pathErr) + return resolved{settings: settings, err: err, path: path, pathErr: pathErr, redact: redact.New(settings.APIKey, c.apiKey, envKey)} +} + +// connect resolves the key and URL and builds a client. Errors are +// configuration errors (exit 2); they never contain the key. +func (c *commonFlags) connect(env Env) (*session, error) { + r := c.resolve(env) + settings, red := r.settings, r.redact + if r.err != nil { + if errors.Is(r.err, config.ErrNoAPIKey) && r.pathErr != nil { + return nil, fmt.Errorf("%w (%v)", r.err, r.pathErr) } - return nil, errors.New(red.Error(err)) + return nil, errors.New(red.Error(r.err)) } if settings.LooseConfig { - fmt.Fprintf(env.Stderr, "warning: %s can be read by other users; run: chmod 600 %q\n", path, path) + fmt.Fprintf(env.Stderr, "warning: %s can be read by other users; run: chmod 600 %q\n", r.path, r.path) } client, err := newClient(env, settings.APIURL, settings.APIKey) if err != nil { diff --git a/internal/cli/url.go b/internal/cli/url.go index be4068f..66a21ae 100644 --- a/internal/cli/url.go +++ b/internal/cli/url.go @@ -11,12 +11,15 @@ import ( "net/http" "os" "regexp" + "strconv" "strings" "sync" + "unicode" "github.com/Codeinwp/optimole-cli/internal/api" "github.com/Codeinwp/optimole-cli/internal/imgurl" "github.com/Codeinwp/optimole-cli/internal/output" + "github.com/Codeinwp/optimole-cli/internal/redact" "github.com/Codeinwp/optimole-cli/internal/termecho" ) @@ -64,7 +67,8 @@ Files that are not images (their URLs have /files/) are printed as they are; image flags are an error for them. Exit codes: 0 every input printed, 1 an input failed (the others are still -printed), 2 usage or configuration error, 130 interrupted. +printed) or the API key was rejected, 2 usage or configuration error, 130 +interrupted. A rejected or missing API key stops the run at the first key. ` + commonUsage const ( @@ -74,7 +78,9 @@ const ( maxURLLine = 1 << 20 ) -var keyPattern = regexp.MustCompile(`^[0-9a-fA-F]{32}$`) +// keyLike matches a line that could be an API key given by mistake: it is +// never echoed. +var keyLike = regexp.MustCompile(`^[A-Za-z0-9_-]{20,}$`) // urlEdits is what the flags change in every URL. type urlEdits struct { @@ -97,7 +103,6 @@ func (e urlEdits) apply(u *imgurl.URL) { } } -// urlFlags are the flags of "optimole url". type urlFlags struct { width, height, resize, gravity, focus, quality, format, dpr string download, reset bool @@ -237,7 +242,7 @@ func runURL(ctx context.Context, args []string, env Env) int { // The first key connects inside a lookup, whose warnings would otherwise // race the results written to stderr. env.Stderr = &lockedWriter{w: env.Stderr} - run := &urlRun{env: env, common: &common, edits: edits} + run := &urlRun{env: env, common: &common, edits: edits, redact: common.resolve(env).redact} return run.run(ctx, inputs, concurrency, jsonOut) } @@ -299,10 +304,12 @@ func classify(line string) urlInput { return urlInput{skip: true} case strings.HasPrefix(s, "{"): return classifyJSON(s) - case keyPattern.MatchString(s): + case api.IsAssetKey(s): return urlInput{shown: s, key: strings.ToLower(s)} case hasHTTPScheme(s): return urlInput{shown: s, url: s} + case keyLike.MatchString(s): + return urlInput{shown: "(hidden)", err: errors.New("not an asset key or an Optimole URL; it looks like an API key, so it is not shown")} } return urlInput{shown: shorten(s), err: errors.New("not an asset key (32 hexadecimal characters) or an Optimole URL")} } @@ -361,7 +368,9 @@ type urlResult struct { err error code string // fatal, when not zero, stops the run with this exit code after - // printing fatalMsg: no later input can succeed. + // printing fatalMsg. It is set when the API key is missing or rejected, + // which would fail every later key the same way; later inputs are not + // read. fatal int fatalMsg string } @@ -371,6 +380,7 @@ type urlRun struct { env Env common *commonFlags edits urlEdits + redact redact.Redactor // hides the known API keys in what is printed once sync.Once sess *session @@ -424,7 +434,17 @@ func (r *urlRun) run(ctx context.Context, inputs func(context.Context) (<-chan s return ExitInterrupted } failed := 0 - for ch := range results { + for { + var ch chan urlResult + var more bool + select { + case ch, more = <-results: + case <-ctx.Done(): + return interrupted() + } + if !more { + break + } var res urlResult select { case res = <-ch: @@ -442,13 +462,13 @@ func (r *urlRun) run(ctx context.Context, inputs func(context.Context) (<-chan s if res.err != nil { failed++ if jsonOut { - enc.Encode(urlLine{Input: res.in.shown, Error: res.err.Error(), Code: res.code}) + enc.Encode(urlLine{Input: r.redact.String(res.in.shown), Error: r.redact.String(res.err.Error()), Code: res.code}) } else { - fmt.Fprintf(r.env.Stderr, "optimole url: %s: %v\n", res.in.shown, res.err) + fmt.Fprintf(r.env.Stderr, "optimole url: %s: %s\n", r.printable(res.in.shown), r.printable(res.err.Error())) } } else if jsonOut { enc.Encode(urlLine{ - Input: res.in.shown, URL: res.url, Key: res.key, MimeType: res.in.mimeType, + Input: r.redact.String(res.in.shown), URL: res.url, Key: res.key, MimeType: res.in.mimeType, OriginalWidth: res.in.width, OriginalHeight: res.in.height, }) } else { @@ -471,6 +491,16 @@ func (r *urlRun) run(ctx context.Context, inputs func(context.Context) (<-chan s return ExitOK } +// printable hides the known API keys in s and quotes it when it holds +// control characters, which a terminal would otherwise act on. +func (r *urlRun) printable(s string) string { + s = r.redact.String(s) + if strings.ContainsFunc(s, unicode.IsControl) { + return strconv.Quote(s) + } + return s +} + // urlLine is the JSON object printed for each input. type urlLine struct { Input string `json:"input"` @@ -515,11 +545,12 @@ func (r *urlRun) resolve(ctx context.Context, in urlInput) urlResult { return res } r.edits.apply(u) - res.url = u.String() - if res.key == "" { - res.key = in.key + if err := u.Validate(); err != nil { + res.err, res.code = err, codeInvalidInput + return res } - if res.key == "" && keyPattern.MatchString(u.ID()) { + res.url = u.String() + if res.key == "" && api.IsAssetKey(u.ID()) { res.key = strings.ToLower(u.ID()) } return res diff --git a/internal/cli/url_test.go b/internal/cli/url_test.go index 4583896..ff0e10f 100644 --- a/internal/cli/url_test.go +++ b/internal/cli/url_test.go @@ -2,7 +2,10 @@ package cli import ( "context" + "fmt" + "io" "net/http" + "strconv" "strings" "sync" "testing" @@ -255,6 +258,9 @@ func TestURLUsageErrors(t *testing.T) { {"--set", "id:" + urlHash}, {"--set", "nocolon"}, {"--set", "bl:1/2"}, + {"--set", "g:fp:0,1"}, + {"--set", "cb:x:abc.d"}, + {"--focus", "NaN:0"}, {"--width", "0"}, {"--width", ""}, {"--resize", "crop"}, @@ -311,3 +317,132 @@ func TestURLHelp(t *testing.T) { t.Fatalf("exit %d, help: %s", code, h.stderr.String()) } } + +func TestURLInterruptedWhileStdinIsIdle(t *testing.T) { + h := newHarness(t) + pr, pw := io.Pipe() + defer pw.Close() + h.stdin = pr + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(50 * time.Millisecond) + cancel() + }() + done := make(chan int, 1) + go func() { done <- h.runCtx(ctx, "url") }() + select { + case code := <-done: + if code != ExitInterrupted { + t.Fatalf("exit %d, want 130", code) + } + case <-time.After(5 * time.Second): + t.Fatal("Ctrl-C did not stop a run waiting for stdin") + } +} + +func TestURLLimitsParallelLookups(t *testing.T) { + for _, limit := range []int{1, 2} { + h := newHarness(t).withKey() + var keys []string + for i := 0; i < 6; i++ { + keys = append(keys, h.srv.AddAsset(testserver.Asset{Filename: fmt.Sprintf("%d.jpg", i), ContentType: "image/jpeg"})) + } + var mu sync.Mutex + inside, most := 0, 0 + // Each lookup waits until limit lookups are inside, so a limit that + // lets more through shows up as more at once. + h.srv.Hook = func(route testserver.Route, _ *http.Request) { + if route != testserver.RouteAsset { + return + } + mu.Lock() + inside++ + most = max(most, inside) + mu.Unlock() + for deadline := time.Now().Add(2 * time.Second); time.Now().Before(deadline); time.Sleep(time.Millisecond) { + mu.Lock() + n := inside + mu.Unlock() + if n >= limit { + break + } + } + time.Sleep(20 * time.Millisecond) + mu.Lock() + inside-- + mu.Unlock() + } + if code := h.run(append([]string{"url", "-c", strconv.Itoa(limit)}, keys...)...); code != ExitOK { + t.Fatalf("-c %d: exit %d: %s", limit, code, h.stderr.String()) + } + if most != limit { + t.Fatalf("-c %d: at most %d lookups at once, want %d", limit, most, limit) + } + } +} + +func TestURLReportsOutputFailures(t *testing.T) { + h := newHarness(t) + env := h.env() + env.Stdout = &failingWriter{} + if code := Run(context.Background(), []string{"url", urlImage, urlImage}, env); code != ExitFailure { + t.Fatalf("exit %d, want 1", code) + } + if !strings.Contains(h.stderr.String(), "writing output: no space left on device") { + t.Fatalf("stderr: %s", h.stderr.String()) + } +} + +func TestURLRefusesNewlinesInURLs(t *testing.T) { + h := newHarness(t) + injected := urlImage + "\nhttps://evil.example/x.jpg" + h.stdin = strings.NewReader(`{"url":"` + strings.ReplaceAll(injected, "\n", `\n`) + `"}` + "\n") + if code := h.run("url"); code != ExitFailure { + t.Fatalf("exit %d, want 1", code) + } + if h.stdout.Len() != 0 || strings.Contains(h.stderr.String(), "\nhttps://evil") { + t.Fatalf("stdout %q, stderr %q: the newline went through", h.stdout.String(), h.stderr.String()) + } + if code := h.run("url", injected); code != ExitFailure || h.stdout.Len() != 0 { + t.Fatalf("argument: exit %d, stdout %q", code, h.stdout.String()) + } +} + +func TestURLDoesNotEchoAPIKeys(t *testing.T) { + h := newHarness(t) + secret := "my.saved.secret.key" + h.vars[config.EnvAPIKey] = secret + if code := h.run("url", secret, "https://example.com/x/"+secret, "--json"); code != ExitFailure { + t.Fatalf("exit %d, want 1", code) + } + if out := h.stdout.String() + h.stderr.String(); strings.Contains(out, secret) || !strings.Contains(out, "[REDACTED]") { + t.Fatalf("the configured key is printed:\n%s", out) + } + if code := h.run("url", secret, "https://example.com/x/"+secret); code != ExitFailure { + t.Fatalf("exit %d, want 1", code) + } + if out := h.stdout.String() + h.stderr.String(); strings.Contains(out, secret) || strings.Count(out, "[REDACTED]") != 2 { + t.Fatalf("the configured key is printed:\n%s", out) + } + scoped :="optml-s" + strings.Repeat("Ab3", 19) + if code := h.run("url", scoped); code != ExitFailure { + t.Fatalf("exit %d, want 1", code) + } + if strings.Contains(h.stderr.String(), scoped) || !strings.Contains(h.stderr.String(), "looks like an API key") { + t.Fatalf("stderr: %s", h.stderr.String()) + } +} + +func TestURLRefusesURLsTheCDNWouldMisread(t *testing.T) { + h := newHarness(t) + raw := "https://mlabc.i.optimole.com/w:300/https://example.com/w:9/a.jpg" + if code := h.run("url", raw, "--width", "100"); code != ExitOK { + t.Fatalf("exit %d: %s", code, h.stderr.String()) + } + if code := h.run("url", raw, "--unset", "w", "--json"); code != ExitFailure { + t.Fatalf("exit %d, want 1", code) + } + if got := h.jsonLines()[0]; got["code"] != "invalid_input" || !strings.Contains(got["error"].(string), `"w:9"`) { + t.Fatalf("line = %v", got) + } +} diff --git a/internal/imgurl/imgurl.go b/internal/imgurl/imgurl.go index a74926f..09729e8 100644 --- a/internal/imgurl/imgurl.go +++ b/internal/imgurl/imgurl.go @@ -16,6 +16,7 @@ import ( "errors" "fmt" "strings" + "unicode" ) // ErrNotOptimole is returned for URLs that are not Optimole delivery URLs. @@ -62,16 +63,22 @@ type URL struct { // defaultImageOptions are the options of an image URL from the dashboard. var defaultImageOptions = []Option{{"w", "auto"}, {"h", "auto"}, {"q", "auto"}} -// Parse splits an Optimole URL. The host must be an Optimole one, or the -// path must hold an "id:" source or CDN options (custom domains). +// Parse splits an Optimole URL. The host must be an Optimole one, or (for +// custom domains) the path must hold an "id:" source or CDN options, or start +// with the origin URL. func Parse(raw string) (*URL, error) { s := strings.TrimSpace(raw) + // A URL never holds spaces or control characters; a newline would also + // turn one printed URL into two. + if strings.ContainsFunc(s, func(r rune) bool { return unicode.IsSpace(r) || unicode.IsControl(r) }) { + return nil, fmt.Errorf("%w: it contains spaces or control characters", ErrNotOptimole) + } scheme, rest, ok := cutScheme(s) if !ok { return nil, fmt.Errorf("%w: it must start with https://", ErrNotOptimole) } host, path, _ := strings.Cut(rest, "/") - if host == "" || strings.ContainsAny(host, "@?#\\ \t") { + if host == "" || strings.ContainsAny(host, "@?#\\") { return nil, fmt.Errorf("%w: invalid host %q", ErrNotOptimole, host) } path, suffix := cutSuffix(path) @@ -96,7 +103,7 @@ func Parse(raw string) (*URL, error) { } u.extra = append(u.extra, seg) } - if !isOptimoleHost(host) && len(u.options) == 0 && u.ID() == "" { + if !isOptimoleHost(host) && start > 0 && len(u.options) == 0 && u.ID() == "" { return nil, fmt.Errorf("%w: %s is not an Optimole host and the path has no CDN options", ErrNotOptimole, host) } return u, nil @@ -106,8 +113,9 @@ func Parse(raw string) (*URL, error) { // lower case. func cutScheme(s string) (scheme, rest string, ok bool) { for _, sc := range []string{"https", "http"} { - if len(s) > len(sc)+3 && strings.EqualFold(s[:len(sc)+3], sc+"://") { - return sc, s[len(sc)+3:], true + prefix := sc + "://" + if len(s) > len(prefix) && strings.EqualFold(s[:len(prefix)], prefix) { + return sc, s[len(prefix):], true } } return "", "", false @@ -150,6 +158,25 @@ func (u *URL) ID() string { return id } +// Validate reports a URL the CDN would misread. The CDN looks for options in +// the whole path, the origin's own path included, so a segment there such as +// "w:9" counts as an option unless the URL already sets that option before +// the source. +func (u *URL) Validate() error { + segs := strings.Split(u.source, "/") + for _, seg := range segs[1:] { + name, _, ok := strings.Cut(seg, ":") + if !ok || !(IsOption(name) || name == "id") { + continue + } + if _, set := u.Get(name); set || (name == "id" && u.ID() != "") { + continue + } + return fmt.Errorf("the origin path has %q, which the CDN reads as an option, so it would misread this URL", seg) + } + return nil +} + // Kind tells whether the URL is an image the CDN can transform. func (u *URL) Kind() Kind { for _, e := range u.extra { diff --git a/internal/imgurl/imgurl_test.go b/internal/imgurl/imgurl_test.go index 4c30e0c..b29a801 100644 --- a/internal/imgurl/imgurl_test.go +++ b/internal/imgurl/imgurl_test.go @@ -117,6 +117,12 @@ func TestParseReadsPartsLikeTheCDN(t *testing.T) { kind: Image, out: "https://cdn.example.com/w:640/https://example.com/a.jpg", }, + { + name: "a custom domain whose path starts with the origin URL", + raw: "https://cdn.example.com/https://example.com/a.jpg", + kind: Image, + out: "https://cdn.example.com/https://example.com/a.jpg", + }, { name: "upper-case scheme", raw: "HTTPS://mlabc.i.optimole.com/id:" + hash + "/directUpload/a.jpg", @@ -156,6 +162,11 @@ func TestParseRefusesOtherURLs(t *testing.T) { "https://mlabc.i.optimole.com/id:/directUpload/a.jpg", "https://web.archive.org/web/2020/https://example.com/a.jpg", "https://example.com/images/https://example.org/a.jpg", + // Spaces and control characters: a newline would print two URLs. + "https://mlabc.i.optimole.com/w:100/https://example.com/a.jpg\nhttps://evil.example/x", + "https://mlabc.i.optimole.com/w:100/https://example.com/a b.jpg", + "https://mlabc.i.optimole.com/w:100/https://example.com/a\tb.jpg", + "https://mlabc.i.optimole.com/w:100/https://example.com/a\x1b[31m.jpg", } { if u, err := Parse(raw); !errors.Is(err, ErrNotOptimole) { t.Errorf("Parse(%q) = %v, %v; want ErrNotOptimole", raw, u, err) @@ -200,6 +211,33 @@ func TestUnsetAndReset(t *testing.T) { } } +func TestValidateFindsOptionsTheCDNReadsInTheOriginPath(t *testing.T) { + tests := []struct { + raw string + bad bool + }{ + {"https://mlabc.i.optimole.com/w:300/https://example.com/w:9/a.jpg", false}, // w is set: the CDN ignores w:9 + {"https://mlabc.i.optimole.com/q:60/https://example.com/w:9/a.jpg", true}, + {"https://mlabc.i.optimole.com/w:300/id:" + hash + "/https://example.com/bl:9/a.jpg", true}, + {"https://mlabc.i.optimole.com/w:300/https://example.com/id:9/a.jpg", true}, + {"https://mlabc.i.optimole.com/w:300/id:" + hash + "/directUpload/id:x/a.jpg", false}, + {"https://mlabc.i.optimole.com/w:300/https://example.com:8443/x:9/a.jpg", false}, + {libImage, false}, + } + for _, tt := range tests { + err := mustParse(t, tt.raw).Validate() + if (err != nil) != tt.bad { + t.Errorf("Validate(%q) = %v, want an error: %v", tt.raw, err, tt.bad) + } + } + // Removing the option that shadowed it makes the URL one the CDN misreads. + u := mustParse(t, "https://mlabc.i.optimole.com/w:300/https://example.com/w:9/a.jpg") + u.Unset("w") + if u.Validate() == nil { + t.Fatal("Validate accepted a URL whose only w: is in the origin path") + } +} + func TestStringReparsesToTheSameURL(t *testing.T) { for _, raw := range []string{ libImage, @@ -239,5 +277,18 @@ func FuzzParse(f *testing.F) { if !reflect.DeepEqual(u.Options(), v.Options()) || u.ID() != v.ID() || u.Kind() != v.Kind() { t.Fatalf("reparsing %q changed it: %v %q %v -> %v %q %v", s, u.Options(), u.ID(), u.Kind(), v.Options(), v.ID(), v.Kind()) } + // An edited URL parses back with the edits. + v.Set("w", "123") + v.Unset("q") + e, err := Parse(v.String()) + if err != nil { + t.Fatalf("the edited %q does not parse: %v", v.String(), err) + } + if w, _ := e.Get("w"); w != "123" { + t.Fatalf("edit lost: %q has w=%q", v.String(), w) + } + if _, ok := e.Get("q"); ok { + t.Fatalf("edit lost: %q still has q", v.String()) + } }) } diff --git a/internal/imgurl/options.go b/internal/imgurl/options.go index 981509a..fbf906a 100644 --- a/internal/imgurl/options.go +++ b/internal/imgurl/options.go @@ -3,6 +3,7 @@ package imgurl import ( "errors" "fmt" + "math" "strconv" "strings" "unicode" @@ -151,19 +152,32 @@ func Focus(v string) (string, error) { } func fraction(s string) (string, error) { - f, err := strconv.ParseFloat(s, 64) + f, err := finite(s) if err != nil || f < 0 || f > 1 { return "", errors.New("not a fraction") } return strconv.FormatFloat(f, 'f', -1, 64), nil } +// finite parses a number, refusing NaN and infinities. +func finite(s string) (float64, error) { + f, err := strconv.ParseFloat(s, 64) + if err != nil || math.IsNaN(f) || math.IsInf(f, 0) { + return 0, errors.New("not a number") + } + return f, nil +} + // checkGravityValue accepts a gravity code with optional X:Y offsets in // pixels, "obj" (object detection), or a focus point "fp:X:Y". The CDN turns // an unknown code into the center. func checkGravityValue(v string) error { code, rest, hasRest := strings.Cut(v, ":") if code == "fp" { + // The CDN reads the point only in this form: fp:X:Y. + if strings.Contains(rest, ",") { + return errors.New("must be fp:X:Y, with colons") + } _, err := Focus(rest) return err } @@ -178,8 +192,8 @@ func checkGravityValue(v string) error { return nil } xs, ys, ok := strings.Cut(rest, ":") - _, errX := strconv.ParseFloat(xs, 64) - _, errY := strconv.ParseFloat(ys, 64) + _, errX := finite(xs) + _, errY := finite(ys) if !ok || errX != nil || errY != nil { return errors.New("gravity offsets must be two numbers: " + code + ":X:Y") } @@ -223,21 +237,18 @@ func DPR(v string) (string, error) { return v, nil } -// checkCacheBuster applies the CDN's rule: it drops a cache buster unless it -// has at least 3 characters, a dot, and at least 1 more character (cache -// tokens from the dashboard look like "aB3d.1f"). +// checkCacheBuster applies the CDN's rule: it reads the value up to a colon, +// and drops it unless it has at least 3 characters, a dot, and at least 1 +// more character (cache tokens from the dashboard look like "aB3d.1f"). func checkCacheBuster(v string) error { parts := strings.Split(v, ".") - if len(parts) < 2 || len(parts[0]) < 3 || len(parts[1]) < 1 { + if strings.Contains(v, ":") || len(parts) < 2 || len(parts[0]) < 3 || len(parts[1]) < 1 { return errors.New(`must look like "abc.1" (3 or more characters, a dot, then more); the CDN ignores any other value`) } return nil } -// list joins values as "a, b or c". +// list joins two or more values as "a, b or c". func list(values []string) string { - if len(values) < 2 { - return strings.Join(values, "") - } return strings.Join(values[:len(values)-1], ", ") + " or " + values[len(values)-1] } diff --git a/internal/imgurl/options_test.go b/internal/imgurl/options_test.go index 38275b4..e5ce0e9 100644 --- a/internal/imgurl/options_test.go +++ b/internal/imgurl/options_test.go @@ -29,7 +29,7 @@ func TestTypedValues(t *testing.T) { []string{"", "middle", "north", "fp", "obj"}}, {"Focus", Focus, map[string]string{"0.5:0.2": "fp:0.5:0.2", "0,1": "fp:0:1", "1:0.25": "fp:1:0.25", ".5:.5": "fp:0.5:0.5"}, - []string{"", "0.5", "1.5:0", "0:-0.1", "a:b", "0.5:0.5:0.5"}}, + []string{"", "0.5", "1.5:0", "0:-0.1", "a:b", "0.5:0.5:0.5", "NaN:0", "0:NaN", "Inf:0"}}, {"Format", Format, map[string]string{"auto": "", "webp": "webp", "JPG": "jpg", "jpeg": "jpg", "png": "png", "gif": "gif", "ico": "ico", "jxl": "jxl", "heic": "heic"}, @@ -76,7 +76,8 @@ func TestCheck(t *testing.T) { {"bl", ""}, {"bl", "5/6"}, {"bl", "5 6"}, {"bl", "a?b"}, {"bl", "a#b"}, {"bl", "50%"}, {"bl", "a\x00"}, {"w", "0"}, {"h", "x"}, {"q", "200"}, {"rt", "crop"}, {"dpr", "9"}, {"g", "middle"}, {"g", "fp"}, {"g", "fp:2:0"}, {"g", "ce:1"}, {"g", "ce:a:b"}, - {"cb", "abc123xyz789"}, {"cb", "ab.c"}, {"cb", "abc."}, {"cb", ".abc"}, + {"g", "fp:0,1"}, {"g", "fp:NaN:0"}, {"g", "ce:NaN:0"}, {"g", "ce:Inf:0"}, + {"cb", "abc123xyz789"}, {"cb", "ab.c"}, {"cb", "abc."}, {"cb", ".abc"}, {"cb", "x:abc.d"}, {"cb", "abc.d:x"}, } for _, b := range bad { if err := Check(b[0], b[1]); err == nil { diff --git a/internal/testserver/testserver.go b/internal/testserver/testserver.go index 3675dde..9e0277f 100644 --- a/internal/testserver/testserver.go +++ b/internal/testserver/testserver.go @@ -601,8 +601,6 @@ func (s *Server) AddAsset(a Asset) string { var assetPath = regexp.MustCompile(`^/api/assets/([0-9a-fA-F]{32})$`) -// handleAssetPath sends GET /assets/{key} to handleGetAsset and everything -// else under /assets/ to handleComplete. func (s *Server) handleAssetPath(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodGet && assetPath.MatchString(r.URL.Path) { s.handleGetAsset(w, r) @@ -618,13 +616,14 @@ func (s *Server) handleGetAsset(w http.ResponseWriter, r *http.Request) { status := 0 var drop func() defer func() { s.finishThenDrop(ev, status, drop) }() - key := strings.ToLower(assetPath.FindStringSubmatch(r.URL.Path)[1]) + sent := assetPath.FindStringSubmatch(r.URL.Path)[1] + key := strings.ToLower(sent) ev.Key = key if injected != nil { status, drop = writeInjected(w, injected) return } - if key != assetPath.FindStringSubmatch(r.URL.Path)[1] { + if key != sent { s.violate("GET %s: the key is not lower case", r.URL.Path) } k, ok := s.authenticate(w, r) From 560801ac68c51c0a3fb01b66e6e4c6be88b59b53 Mon Sep 17 00:00:00 2001 From: selul Date: Thu, 5 Nov 2026 11:56:34 +0200 Subject: [PATCH 06/11] url: fix the second review's findings - Usage errors hide the API keys in use, and a long input line is shortened only after its keys are hidden, so no part of a key shows. - Validate follows the CDN's literal rules: it deletes the first /ig:avif and /f:avif in the path, and reads no other ig value. - Before an origin URL, the segments the CDN removes are dropped, so a custom-domain URL left without options pipes back. - The dispatcher stops waiting for stdin when the run ends. - gofmt on the test file, which CI caught. Co-Authored-By: Claude Opus 5.5 --- internal/cli/url.go | 36 ++++++++++++++++++++--------- internal/cli/url_test.go | 35 +++++++++++++++++++++++++++- internal/imgurl/imgurl.go | 42 +++++++++++++++++++++++----------- internal/imgurl/imgurl_test.go | 20 ++++++++++++++++ 4 files changed, 108 insertions(+), 25 deletions(-) diff --git a/internal/cli/url.go b/internal/cli/url.go index 66a21ae..ae74b7f 100644 --- a/internal/cli/url.go +++ b/internal/cli/url.go @@ -214,11 +214,14 @@ func runURL(ctx context.Context, args []string, env Env) int { if err != nil { return flagExit(err) } + // Flag values are echoed in usage errors: a key given by mistake must + // not be. + red := common.resolve(env).redact visited := map[string]bool{} fs.Visit(func(fl *flag.Flag) { visited[fl.Name] = true }) edits, err := f.edits(visited) if err != nil { - return usageError(env, "url", "%v", err) + return usageError(env, "url", "%s", red.String(err.Error())) } if concurrency < 1 || concurrency > urlMaxConcurrency { return usageError(env, "url", "--concurrency must be between 1 and %d", urlMaxConcurrency) @@ -242,7 +245,7 @@ func runURL(ctx context.Context, args []string, env Env) int { // The first key connects inside a lookup, whose warnings would otherwise // race the results written to stderr. env.Stderr = &lockedWriter{w: env.Stderr} - run := &urlRun{env: env, common: &common, edits: edits, redact: common.resolve(env).redact} + run := &urlRun{env: env, common: &common, edits: edits, redact: red} return run.run(ctx, inputs, concurrency, jsonOut) } @@ -311,7 +314,7 @@ func classify(line string) urlInput { case keyLike.MatchString(s): return urlInput{shown: "(hidden)", err: errors.New("not an asset key or an Optimole URL; it looks like an API key, so it is not shown")} } - return urlInput{shown: shorten(s), err: errors.New("not an asset key (32 hexadecimal characters) or an Optimole URL")} + return urlInput{shown: s, err: errors.New("not an asset key (32 hexadecimal characters) or an Optimole URL")} } // classifyJSON reads a line of "optimole upload --json" or "optimole url --json". @@ -326,7 +329,7 @@ func classifyJSON(s string) urlInput { Summary json.RawMessage `json:"summary"` } if err := json.Unmarshal([]byte(s), &line); err != nil { - return urlInput{shown: shorten(s), err: errors.New("not valid JSON")} + return urlInput{shown: s, err: errors.New("not valid JSON")} } if line.Summary != nil { return urlInput{skip: true} @@ -334,7 +337,7 @@ func classifyJSON(s string) urlInput { if line.URL != "" { return urlInput{shown: line.URL, url: line.URL, mimeType: line.MimeType, width: line.OriginalWidth, height: line.OriginalHeight} } - in := urlInput{shown: shorten(s)} + in := urlInput{shown: s} if line.Path != "" { in.shown = line.Path } @@ -368,9 +371,10 @@ type urlResult struct { err error code string // fatal, when not zero, stops the run with this exit code after - // printing fatalMsg. It is set when the API key is missing or rejected, - // which would fail every later key the same way; later inputs are not - // read. + // printing fatalMsg: nothing more is printed, and the lookups still + // running are cancelled. It is set when connecting fails (no API key, a + // bad API URL) or the API key is rejected, which would fail every later + // key the same way. fatal int fatalMsg string } @@ -403,7 +407,17 @@ func (r *urlRun) run(ctx context.Context, inputs func(context.Context) (<-chan s go func() { defer close(results) sem := make(chan struct{}, concurrency) - for line := range lines { + for { + var line string + var more bool + select { + case line, more = <-lines: + case <-runCtx.Done(): + return + } + if !more { + return + } in := classify(line) if in.skip { continue @@ -462,9 +476,9 @@ func (r *urlRun) run(ctx context.Context, inputs func(context.Context) (<-chan s if res.err != nil { failed++ if jsonOut { - enc.Encode(urlLine{Input: r.redact.String(res.in.shown), Error: r.redact.String(res.err.Error()), Code: res.code}) + enc.Encode(urlLine{Input: shorten(r.redact.String(res.in.shown)), Error: r.redact.String(res.err.Error()), Code: res.code}) } else { - fmt.Fprintf(r.env.Stderr, "optimole url: %s: %s\n", r.printable(res.in.shown), r.printable(res.err.Error())) + fmt.Fprintf(r.env.Stderr, "optimole url: %s: %s\n", r.printable(shorten(r.redact.String(res.in.shown))), r.printable(res.err.Error())) } } else if jsonOut { enc.Encode(urlLine{ diff --git a/internal/cli/url_test.go b/internal/cli/url_test.go index ff0e10f..d979a75 100644 --- a/internal/cli/url_test.go +++ b/internal/cli/url_test.go @@ -424,7 +424,7 @@ func TestURLDoesNotEchoAPIKeys(t *testing.T) { if out := h.stdout.String() + h.stderr.String(); strings.Contains(out, secret) || strings.Count(out, "[REDACTED]") != 2 { t.Fatalf("the configured key is printed:\n%s", out) } - scoped :="optml-s" + strings.Repeat("Ab3", 19) + scoped := "optml-s" + strings.Repeat("Ab3", 19) if code := h.run("url", scoped); code != ExitFailure { t.Fatalf("exit %d, want 1", code) } @@ -446,3 +446,36 @@ func TestURLRefusesURLsTheCDNWouldMisread(t *testing.T) { t.Fatalf("line = %v", got) } } + +func TestURLRedactsKeysInEveryMessage(t *testing.T) { + h := newHarness(t) + secret := "my.saved.secret.key" + h.vars[config.EnvAPIKey] = secret + // In a usage error. + if code := h.run("url", urlImage, "--width", secret); code != ExitUsage { + t.Fatalf("exit %d, want 2", code) + } + if strings.Contains(h.stderr.String(), secret) || !strings.Contains(h.stderr.String(), "[REDACTED]") { + t.Fatalf("usage error: %s", h.stderr.String()) + } + // In an error's own text, in both output modes. + withCredentials := "https://" + secret + "@mlabc.i.optimole.com/id:" + urlHash + "/a.jpg" + for _, args := range [][]string{{"url", withCredentials}, {"url", withCredentials, "--json"}} { + if code := h.run(args...); code != ExitFailure { + t.Fatalf("%v: exit %d, want 1", args, code) + } + if out := h.stdout.String() + h.stderr.String(); strings.Contains(out, secret) || !strings.Contains(out, "invalid host") { + t.Fatalf("%v printed the key:\n%s", args, out) + } + } + // In a long line, which is shortened only after the key is hidden. + long := strings.Repeat("x", 110) + " " + secret // cut at 120 characters, inside the key + for _, args := range [][]string{{"url", long}, {"url", long, "--json"}} { + if code := h.run(args...); code != ExitFailure { + t.Fatalf("exit %d, want 1", code) + } + if out := h.stdout.String() + h.stderr.String(); strings.Contains(out, secret[:6]) { + t.Fatalf("%v printed part of the key:\n%s", args[2:], out) + } + } +} diff --git a/internal/imgurl/imgurl.go b/internal/imgurl/imgurl.go index 09729e8..c463f13 100644 --- a/internal/imgurl/imgurl.go +++ b/internal/imgurl/imgurl.go @@ -51,8 +51,9 @@ type URL struct { // Set and Options use its first copy, the one the CDN applies, and // Unset removes every copy. options []Option - // extra are the other segments before the source, kept as they are: - // "files" marks a file URL, and the CDN ignores the rest. + // extra are the other segments before the source: "files" marks a file + // URL, and the CDN ignores the rest. Before an origin URL the CDN removes + // them, so Parse drops them there, "files" aside. extra []string // source is the rest of the path, from "id:" or the origin's scheme. source string @@ -101,7 +102,9 @@ func Parse(raw string) (*URL, error) { u.options = append(u.options, Option{name, value}) continue } - u.extra = append(u.extra, seg) + if u.ID() != "" || seg == "files" { + u.extra = append(u.extra, seg) + } } if !isOptimoleHost(host) && start > 0 && len(u.options) == 0 && u.ID() == "" { return nil, fmt.Errorf("%w: %s is not an Optimole host and the path has no CDN options", ErrNotOptimole, host) @@ -158,21 +161,34 @@ func (u *URL) ID() string { return id } -// Validate reports a URL the CDN would misread. The CDN looks for options in -// the whole path, the origin's own path included, so a segment there such as -// "w:9" counts as an option unless the URL already sets that option before -// the source. +// Validate reports a URL the CDN would misread because of a segment of the +// origin's own path: +// - The CDN looks for options in the whole path, so a segment such as "w:9" +// counts as an option unless the URL already sets that option before the +// source. +// - It deletes the first "/ig:avif" and the first "/f:avif" in the path, so +// one in the origin path is deleted unless the URL's options hold it. func (u *URL) Validate() error { segs := strings.Split(u.source, "/") for _, seg := range segs[1:] { - name, _, ok := strings.Cut(seg, ":") - if !ok || !(IsOption(name) || name == "id") { - continue + name, value, ok := strings.Cut(seg, ":") + var misread bool + switch { + case !ok: + case seg == "ig:avif" || seg == "f:avif": + v, _ := u.Get(name) + misread = v != value + case name == "ig": + // The CDN reads no other "ig" value. + case name == "id": + misread = u.ID() == "" + case IsOption(name): + _, set := u.Get(name) + misread = !set } - if _, set := u.Get(name); set || (name == "id" && u.ID() != "") { - continue + if misread { + return fmt.Errorf("the origin path has %q, which the CDN reads as an option, so it would misread this URL", seg) } - return fmt.Errorf("the origin path has %q, which the CDN reads as an option, so it would misread this URL", seg) } return nil } diff --git a/internal/imgurl/imgurl_test.go b/internal/imgurl/imgurl_test.go index b29a801..5048973 100644 --- a/internal/imgurl/imgurl_test.go +++ b/internal/imgurl/imgurl_test.go @@ -117,6 +117,13 @@ func TestParseReadsPartsLikeTheCDN(t *testing.T) { kind: Image, out: "https://cdn.example.com/w:640/https://example.com/a.jpg", }, + { + name: "before an origin URL, segments the CDN removes are dropped", + raw: "https://cdn.example.com/w:640/extra/files/https://example.com/a.jpg", + options: []Option{{"w", "640"}}, + kind: File, + out: "https://cdn.example.com/w:640/files/https://example.com/a.jpg", + }, { name: "a custom domain whose path starts with the origin URL", raw: "https://cdn.example.com/https://example.com/a.jpg", @@ -222,6 +229,13 @@ func TestValidateFindsOptionsTheCDNReadsInTheOriginPath(t *testing.T) { {"https://mlabc.i.optimole.com/w:300/https://example.com/id:9/a.jpg", true}, {"https://mlabc.i.optimole.com/w:300/id:" + hash + "/directUpload/id:x/a.jpg", false}, {"https://mlabc.i.optimole.com/w:300/https://example.com:8443/x:9/a.jpg", false}, + // The CDN reads no "ig" value but avif, and deletes the first + // "/ig:avif" and "/f:avif" in the path. + {"https://mlabc.i.optimole.com/w:300/https://example.com/uploads/ig:other/a.jpg", false}, + {"https://mlabc.i.optimole.com/w:300/https://example.com/uploads/ig:avif/a.jpg", true}, + {"https://mlabc.i.optimole.com/w:300/ig:avif/https://example.com/uploads/ig:avif/a.jpg", false}, + {"https://mlabc.i.optimole.com/f:best/https://example.com/f:avif/a.jpg", true}, + {"https://mlabc.i.optimole.com/f:avif/https://example.com/f:avif/a.jpg", false}, {libImage, false}, } for _, tt := range tests { @@ -230,6 +244,12 @@ func TestValidateFindsOptionsTheCDNReadsInTheOriginPath(t *testing.T) { t.Errorf("Validate(%q) = %v, want an error: %v", tt.raw, err, tt.bad) } } + // A custom-domain URL left without options still parses back. + c := mustParse(t, "https://cdn.example.com/w:640/extra/https://example.com/a.jpg") + c.Unset("w") + if got := mustParse(t, c.String()).String(); got != "https://cdn.example.com/https://example.com/a.jpg" { + t.Fatalf("round trip = %q", got) + } // Removing the option that shadowed it makes the URL one the CDN misreads. u := mustParse(t, "https://mlabc.i.optimole.com/w:300/https://example.com/w:9/a.jpg") u.Unset("w") From 89988597887fd8f9cecc12a91b74b6bdd9011696 Mon Sep 17 00:00:00 2001 From: selul Date: Thu, 5 Nov 2026 12:02:51 +0200 Subject: [PATCH 07/11] url: follow the CDN's text deletions and hide keys in flag errors - Validate works on the written URL like the CDN: it refuses a URL whose first /ig:avif or /f:avif (deleted as text, not as a segment) falls in the origin path, or whose origin path alone holds ig:avif, which turns AVIF off. Checked against the viewer-request function on 7 URLs. - Before an origin URL, a segment holding ig:avif is kept: dropping it would turn AVIF back on. - The flag package's own errors (a bad -c or --json value) hide the keys known before the flags are read. Co-Authored-By: Claude Opus 5.5 --- internal/cli/url.go | 16 +++++++++++ internal/cli/url_test.go | 9 +++++++ internal/imgurl/imgurl.go | 49 +++++++++++++++++++++------------- internal/imgurl/imgurl_test.go | 9 +++++++ 4 files changed, 64 insertions(+), 19 deletions(-) diff --git a/internal/cli/url.go b/internal/cli/url.go index ae74b7f..e8037e7 100644 --- a/internal/cli/url.go +++ b/internal/cli/url.go @@ -191,6 +191,9 @@ func runURL(ctx context.Context, args []string, env Env) int { fs := newFlagSet("url", urlUsage, env) var common commonFlags common.register(fs, true) + // The flag package echoes a bad value; hide the keys known before the + // flags are read (environment and saved key) in what it prints. + fs.SetOutput(redactingWriter{w: env.Stderr, redact: common.resolve(env).redact}) var f urlFlags var jsonOut bool concurrency := urlDefaultConcurrency @@ -249,6 +252,19 @@ func runURL(ctx context.Context, args []string, env Env) int { return run.run(ctx, inputs, concurrency, jsonOut) } +// redactingWriter hides secrets in what it passes on. +type redactingWriter struct { + w io.Writer + redact redact.Redactor +} + +func (r redactingWriter) Write(p []byte) (int, error) { + if _, err := io.WriteString(r.w, r.redact.String(string(p))); err != nil { + return 0, err + } + return len(p), nil +} + // lockedWriter serializes writes from several goroutines. type lockedWriter struct { mu sync.Mutex diff --git a/internal/cli/url_test.go b/internal/cli/url_test.go index d979a75..ee94b0f 100644 --- a/internal/cli/url_test.go +++ b/internal/cli/url_test.go @@ -458,6 +458,15 @@ func TestURLRedactsKeysInEveryMessage(t *testing.T) { if strings.Contains(h.stderr.String(), secret) || !strings.Contains(h.stderr.String(), "[REDACTED]") { t.Fatalf("usage error: %s", h.stderr.String()) } + // In the flag package's own errors. + for _, args := range [][]string{{"url", urlImage, "-c", secret}, {"url", urlImage, "--json=" + secret}} { + if code := h.run(args...); code != ExitUsage { + t.Fatalf("%v: exit %d, want 2", args[2:], code) + } + if strings.Contains(h.stderr.String(), secret) || !strings.Contains(h.stderr.String(), "[REDACTED]") { + t.Fatalf("%v: %s", args[2:], h.stderr.String()) + } + } // In an error's own text, in both output modes. withCredentials := "https://" + secret + "@mlabc.i.optimole.com/id:" + urlHash + "/a.jpg" for _, args := range [][]string{{"url", withCredentials}, {"url", withCredentials, "--json"}} { diff --git a/internal/imgurl/imgurl.go b/internal/imgurl/imgurl.go index c463f13..32d40e3 100644 --- a/internal/imgurl/imgurl.go +++ b/internal/imgurl/imgurl.go @@ -53,7 +53,8 @@ type URL struct { options []Option // extra are the other segments before the source: "files" marks a file // URL, and the CDN ignores the rest. Before an origin URL the CDN removes - // them, so Parse drops them there, "files" aside. + // them, so Parse drops them there, but for "files" and any segment that + // holds "ig:avif", which turns AVIF off wherever it is in the path. extra []string // source is the rest of the path, from "id:" or the origin's scheme. source string @@ -102,7 +103,7 @@ func Parse(raw string) (*URL, error) { u.options = append(u.options, Option{name, value}) continue } - if u.ID() != "" || seg == "files" { + if u.ID() != "" || seg == "files" || strings.Contains(seg, "ig:avif") { u.extra = append(u.extra, seg) } } @@ -161,25 +162,33 @@ func (u *URL) ID() string { return id } -// Validate reports a URL the CDN would misread because of a segment of the -// origin's own path: -// - The CDN looks for options in the whole path, so a segment such as "w:9" -// counts as an option unless the URL already sets that option before the +// Validate reports a URL the CDN would misread because of the origin's own +// path: +// - The CDN deletes the first "/ig:avif" and the first "/f:avif" in the +// path, as text, so a match inside the origin path changes it. +// - "ig:avif" anywhere in the path turns AVIF off, so one in the origin +// path alone does. +// - The CDN looks for options in every segment, so a segment such as +// "w:9" counts as an option unless the URL sets that option before the // source. -// - It deletes the first "/ig:avif" and the first "/f:avif" in the path, so -// one in the origin path is deleted unless the URL's options hold it. func (u *URL) Validate() error { + head := u.head() + path := head + "/" + u.source + for _, cut := range []string{"/ig:avif", "/f:avif"} { + if i := strings.Index(path, cut); i >= len(head) { + return fmt.Errorf("the origin path holds %q, which the CDN deletes, so it would misread this URL", cut) + } + } + if strings.Contains(u.source, "ig:avif") && !strings.Contains(head, "ig:avif") { + return errors.New(`the origin path holds "ig:avif", which turns AVIF off on the CDN, so it would misread this URL`) + } segs := strings.Split(u.source, "/") for _, seg := range segs[1:] { - name, value, ok := strings.Cut(seg, ":") + name, _, ok := strings.Cut(seg, ":") var misread bool switch { - case !ok: - case seg == "ig:avif" || seg == "f:avif": - v, _ := u.Get(name) - misread = v != value - case name == "ig": - // The CDN reads no other "ig" value. + case !ok, name == "ig": + // The CDN reads no "ig" option; "ig:avif" is checked above. case name == "id": misread = u.ID() == "" case IsOption(name): @@ -258,8 +267,13 @@ func (u *URL) Options() []Option { // String returns the URL with its options in the CDN's order, followed by // the other segments and the source. func (u *URL) String() string { + return u.prefix + u.head() + "/" + u.source + u.suffix +} + +// head returns the path before the source: the options, then the other +// segments, each after a slash. +func (u *URL) head() string { var b strings.Builder - b.WriteString(u.prefix) for _, o := range u.Options() { b.WriteByte('/') b.WriteString(o.String()) @@ -268,8 +282,5 @@ func (u *URL) String() string { b.WriteByte('/') b.WriteString(e) } - b.WriteByte('/') - b.WriteString(u.source) - b.WriteString(u.suffix) return b.String() } diff --git a/internal/imgurl/imgurl_test.go b/internal/imgurl/imgurl_test.go index 5048973..748367b 100644 --- a/internal/imgurl/imgurl_test.go +++ b/internal/imgurl/imgurl_test.go @@ -236,6 +236,11 @@ func TestValidateFindsOptionsTheCDNReadsInTheOriginPath(t *testing.T) { {"https://mlabc.i.optimole.com/w:300/ig:avif/https://example.com/uploads/ig:avif/a.jpg", false}, {"https://mlabc.i.optimole.com/f:best/https://example.com/f:avif/a.jpg", true}, {"https://mlabc.i.optimole.com/f:avif/https://example.com/f:avif/a.jpg", false}, + // The deletions match text, not whole segments. + {"https://mlabc.i.optimole.com/w:300/https://example.com/uploads/ig:aviffoo/a.jpg", true}, + {"https://mlabc.i.optimole.com/f:best/https://example.com/uploads/f:aviffoo/a.jpg", true}, + {"https://mlabc.i.optimole.com/ig:aviffoo/https://example.com/uploads/ig:avif/a.jpg", false}, + {"https://mlabc.i.optimole.com/w:300/https://example.com/uploads/xig:avif/a.jpg", true}, {libImage, false}, } for _, tt := range tests { @@ -244,6 +249,10 @@ func TestValidateFindsOptionsTheCDNReadsInTheOriginPath(t *testing.T) { t.Errorf("Validate(%q) = %v, want an error: %v", tt.raw, err, tt.bad) } } + // A segment that turns AVIF off is kept, even before an origin URL. + if got := mustParse(t, "https://cdn.example.com/w:300/xig:avif/https://example.com/a.jpg").String(); got != "https://cdn.example.com/w:300/xig:avif/https://example.com/a.jpg" { + t.Fatalf("String() = %q", got) + } // A custom-domain URL left without options still parses back. c := mustParse(t, "https://cdn.example.com/w:640/extra/https://example.com/a.jpg") c.Unset("w") From 087a3ab131d01a75296bd415071744775d81e527 Mon Sep 17 00:00:00 2001 From: selul Date: Thu, 5 Nov 2026 12:07:29 +0200 Subject: [PATCH 08/11] url: hide a --api-key key in flag errors too The flag package's errors were redacted with the environment and saved keys only, so --api-key K -c K still printed K. The --api-key value is now read from the arguments before they are parsed. Co-Authored-By: Claude Opus 5.5 --- internal/cli/url.go | 30 +++++++++++++++++++++++++++--- internal/cli/url_test.go | 13 +++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/internal/cli/url.go b/internal/cli/url.go index e8037e7..fa29990 100644 --- a/internal/cli/url.go +++ b/internal/cli/url.go @@ -191,9 +191,11 @@ func runURL(ctx context.Context, args []string, env Env) int { fs := newFlagSet("url", urlUsage, env) var common commonFlags common.register(fs, true) - // The flag package echoes a bad value; hide the keys known before the - // flags are read (environment and saved key) in what it prints. - fs.SetOutput(redactingWriter{w: env.Stderr, redact: common.resolve(env).redact}) + // The flag package echoes a bad value; hide the keys in what it prints: + // the environment's, the saved one, and one given with --api-key, read + // here because the flags are not parsed yet. + early := commonFlags{apiKey: apiKeyArg(args)} + fs.SetOutput(redactingWriter{w: env.Stderr, redact: early.resolve(env).redact}) var f urlFlags var jsonOut bool concurrency := urlDefaultConcurrency @@ -252,6 +254,28 @@ func runURL(ctx context.Context, args []string, env Env) int { return run.run(ctx, inputs, concurrency, jsonOut) } +// apiKeyArg returns the value of the last --api-key in args (any form the +// flag package takes: -api-key V, --api-key=V), or "". +func apiKeyArg(args []string) string { + key := "" + for i := 0; i < len(args); i++ { + if args[i] == "--" { + break + } + name, value, hasValue := strings.Cut(strings.TrimLeft(args[i], "-"), "=") + if !strings.HasPrefix(args[i], "-") || name != "api-key" { + continue + } + if hasValue { + key = value + } else if i+1 < len(args) { + key = args[i+1] + i++ + } + } + return key +} + // redactingWriter hides secrets in what it passes on. type redactingWriter struct { w io.Writer diff --git a/internal/cli/url_test.go b/internal/cli/url_test.go index ee94b0f..6ffeffa 100644 --- a/internal/cli/url_test.go +++ b/internal/cli/url_test.go @@ -467,6 +467,19 @@ func TestURLRedactsKeysInEveryMessage(t *testing.T) { t.Fatalf("%v: %s", args[2:], h.stderr.String()) } } + // A key given only with --api-key, repeated as another flag's value. + flagKey := "flag.only.secret.key" + for _, args := range [][]string{ + {"url", urlImage, "--api-key", flagKey, "-c", flagKey}, + {"url", urlImage, "-api-key=" + flagKey, "--json=" + flagKey}, + } { + if code := h.run(args...); code != ExitUsage { + t.Fatalf("%v: exit %d, want 2", args[2:], code) + } + if strings.Contains(h.stderr.String(), flagKey) || !strings.Contains(h.stderr.String(), "[REDACTED]") { + t.Fatalf("%v: %s", args[2:], h.stderr.String()) + } + } // In an error's own text, in both output modes. withCredentials := "https://" + secret + "@mlabc.i.optimole.com/id:" + urlHash + "/a.jpg" for _, args := range [][]string{{"url", withCredentials}, {"url", withCredentials, "--json"}} { From 20eabab61c0ca478449d83849bf1d9522d9a78b9 Mon Sep 17 00:00:00 2001 From: selul Date: Thu, 5 Nov 2026 12:10:10 +0200 Subject: [PATCH 09/11] url: drop the bad value from flag errors instead of guessing keys Reading --api-key before parsing missed keys in several cases (a value taken by another flag, a repeated --api-key, a saved key next to a flag key). The flag package only shows a value inside quotes in its "invalid value" errors, so those messages now leave the value out: no key can reach them, whatever its source. Co-Authored-By: Claude Opus 5.5 --- internal/cli/url.go | 42 +++++++++------------------------------- internal/cli/url_test.go | 34 +++++++++++++++----------------- 2 files changed, 25 insertions(+), 51 deletions(-) diff --git a/internal/cli/url.go b/internal/cli/url.go index fa29990..a605307 100644 --- a/internal/cli/url.go +++ b/internal/cli/url.go @@ -191,11 +191,7 @@ func runURL(ctx context.Context, args []string, env Env) int { fs := newFlagSet("url", urlUsage, env) var common commonFlags common.register(fs, true) - // The flag package echoes a bad value; hide the keys in what it prints: - // the environment's, the saved one, and one given with --api-key, read - // here because the flags are not parsed yet. - early := commonFlags{apiKey: apiKeyArg(args)} - fs.SetOutput(redactingWriter{w: env.Stderr, redact: early.resolve(env).redact}) + fs.SetOutput(flagErrorWriter{w: env.Stderr}) var f urlFlags var jsonOut bool concurrency := urlDefaultConcurrency @@ -254,36 +250,16 @@ func runURL(ctx context.Context, args []string, env Env) int { return run.run(ctx, inputs, concurrency, jsonOut) } -// apiKeyArg returns the value of the last --api-key in args (any form the -// flag package takes: -api-key V, --api-key=V), or "". -func apiKeyArg(args []string) string { - key := "" - for i := 0; i < len(args); i++ { - if args[i] == "--" { - break - } - name, value, hasValue := strings.Cut(strings.TrimLeft(args[i], "-"), "=") - if !strings.HasPrefix(args[i], "-") || name != "api-key" { - continue - } - if hasValue { - key = value - } else if i+1 < len(args) { - key = args[i+1] - i++ - } - } - return key -} +// flagValue matches the value the flag package quotes in its errors: +// `invalid value "x" for flag -c`, `invalid boolean value "x" for -json`. +var flagValue = regexp.MustCompile(`(invalid (?:boolean )?value) "(?:[^"\\]|\\.)*"`) -// redactingWriter hides secrets in what it passes on. -type redactingWriter struct { - w io.Writer - redact redact.Redactor -} +// flagErrorWriter drops the values the flag package quotes in its errors: +// one could be an API key given by mistake, from any source. +type flagErrorWriter struct{ w io.Writer } -func (r redactingWriter) Write(p []byte) (int, error) { - if _, err := io.WriteString(r.w, r.redact.String(string(p))); err != nil { +func (f flagErrorWriter) Write(p []byte) (int, error) { + if _, err := io.WriteString(f.w, flagValue.ReplaceAllString(string(p), "$1")); err != nil { return 0, err } return len(p), nil diff --git a/internal/cli/url_test.go b/internal/cli/url_test.go index 6ffeffa..660d74c 100644 --- a/internal/cli/url_test.go +++ b/internal/cli/url_test.go @@ -458,26 +458,24 @@ func TestURLRedactsKeysInEveryMessage(t *testing.T) { if strings.Contains(h.stderr.String(), secret) || !strings.Contains(h.stderr.String(), "[REDACTED]") { t.Fatalf("usage error: %s", h.stderr.String()) } - // In the flag package's own errors. - for _, args := range [][]string{{"url", urlImage, "-c", secret}, {"url", urlImage, "--json=" + secret}} { - if code := h.run(args...); code != ExitUsage { - t.Fatalf("%v: exit %d, want 2", args[2:], code) - } - if strings.Contains(h.stderr.String(), secret) || !strings.Contains(h.stderr.String(), "[REDACTED]") { - t.Fatalf("%v: %s", args[2:], h.stderr.String()) - } - } - // A key given only with --api-key, repeated as another flag's value. - flagKey := "flag.only.secret.key" - for _, args := range [][]string{ - {"url", urlImage, "--api-key", flagKey, "-c", flagKey}, - {"url", urlImage, "-api-key=" + flagKey, "--json=" + flagKey}, + // In the flag package's own errors, which never show the bad value, + // whatever key it is. + other := "flag.only.secret.key" + for _, tt := range []struct { + args []string + want string + }{ + {[]string{"-c", secret}, "invalid value for flag -c"}, + {[]string{"--json=" + secret}, "invalid boolean value for -json"}, + {[]string{"--api-key", other, "-c", other}, "invalid value for flag -c"}, + {[]string{"--width", "--", "--api-key", other, "-c", other}, "invalid value for flag -c"}, + {[]string{"--api-key", other, "-c", secret, "--api-key", "x"}, "invalid value for flag -c"}, } { - if code := h.run(args...); code != ExitUsage { - t.Fatalf("%v: exit %d, want 2", args[2:], code) + if code := h.run(append([]string{"url", urlImage}, tt.args...)...); code != ExitUsage { + t.Fatalf("%v: exit %d, want 2", tt.args, code) } - if strings.Contains(h.stderr.String(), flagKey) || !strings.Contains(h.stderr.String(), "[REDACTED]") { - t.Fatalf("%v: %s", args[2:], h.stderr.String()) + if got := h.stderr.String(); strings.Contains(got, secret) || strings.Contains(got, other) || !strings.Contains(got, tt.want) { + t.Fatalf("%v: %s", tt.args, got) } } // In an error's own text, in both output modes. From b1fe497ab5db3c628c8c3ec21e36b694603d0f4a Mon Sep 17 00:00:00 2001 From: selul Date: Thu, 5 Nov 2026 12:13:55 +0200 Subject: [PATCH 10/11] url: filter every flag error that repeats what was typed; steady export test - The flag package also repeats a whole argument (bad flag syntax: ---api-key=K) and an unknown flag's name. The first now shows no argument, and a name that looks like an API key is not shown. - export tests: the fake records a download when it ends, and a stalled one can end after its retry (macOS CI did so), so the downloads helper now orders requests by when they started. Co-Authored-By: Claude Opus 5.5 --- internal/cli/url.go | 27 +++++++++++++++++++++------ internal/cli/url_test.go | 3 +++ internal/export/export_test.go | 4 ++++ 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/internal/cli/url.go b/internal/cli/url.go index a605307..86e348b 100644 --- a/internal/cli/url.go +++ b/internal/cli/url.go @@ -250,16 +250,31 @@ func runURL(ctx context.Context, args []string, env Env) int { return run.run(ctx, inputs, concurrency, jsonOut) } -// flagValue matches the value the flag package quotes in its errors: -// `invalid value "x" for flag -c`, `invalid boolean value "x" for -json`. -var flagValue = regexp.MustCompile(`(invalid (?:boolean )?value) "(?:[^"\\]|\\.)*"`) +// The flag package's errors that repeat what was typed: a value +// (`invalid value "x" for flag -c`, `invalid boolean value "x" for -json`), +// a whole argument (`bad flag syntax: ---x`) and a flag name +// (`flag provided but not defined: -x`). +var ( + flagValue = regexp.MustCompile(`(invalid (?:boolean )?value) "(?:[^"\\]|\\.)*"`) + flagSyntax = regexp.MustCompile(`(bad flag syntax): .*`) + flagUnknown = regexp.MustCompile(`(flag provided but not defined: -)(\S+)`) +) -// flagErrorWriter drops the values the flag package quotes in its errors: -// one could be an API key given by mistake, from any source. +// flagErrorWriter drops from the flag package's errors the values and +// arguments typed, and a flag name that looks like an API key: one could be +// a key given by mistake, from any source. type flagErrorWriter struct{ w io.Writer } func (f flagErrorWriter) Write(p []byte) (int, error) { - if _, err := io.WriteString(f.w, flagValue.ReplaceAllString(string(p), "$1")); err != nil { + s := flagValue.ReplaceAllString(string(p), "$1") + s = flagSyntax.ReplaceAllString(s, "$1") + s = flagUnknown.ReplaceAllStringFunc(s, func(m string) string { + if name := flagUnknown.FindStringSubmatch(m)[2]; keyLike.MatchString(name) { + return "flag provided but not defined (it looks like an API key, so it is not shown)" + } + return m + }) + if _, err := io.WriteString(f.w, s); err != nil { return 0, err } return len(p), nil diff --git a/internal/cli/url_test.go b/internal/cli/url_test.go index 660d74c..6177b08 100644 --- a/internal/cli/url_test.go +++ b/internal/cli/url_test.go @@ -470,6 +470,9 @@ func TestURLRedactsKeysInEveryMessage(t *testing.T) { {[]string{"--api-key", other, "-c", other}, "invalid value for flag -c"}, {[]string{"--width", "--", "--api-key", other, "-c", other}, "invalid value for flag -c"}, {[]string{"--api-key", other, "-c", secret, "--api-key", "x"}, "invalid value for flag -c"}, + {[]string{"---api-key=" + secret}, "bad flag syntax"}, + {[]string{"--optml-s" + strings.Repeat("Ab3", 19)}, "it looks like an API key"}, + {[]string{"--widht", "300"}, "flag provided but not defined: -widht"}, } { if code := h.run(append([]string{"url", urlImage}, tt.args...)...); code != ExitUsage { t.Fatalf("%v: exit %d, want 2", tt.args, code) diff --git a/internal/export/export_test.go b/internal/export/export_test.go index 6385032..86467c8 100644 --- a/internal/export/export_test.go +++ b/internal/export/export_test.go @@ -12,6 +12,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "sort" "strings" "sync" "testing" @@ -90,6 +91,8 @@ func (f *fixture) file(rel string) []byte { return b } +// downloads returns the archive requests in the order they started: the fake +// records a request when it ends, and a stalled one can end after its retry. func (f *fixture) downloads() []testserver.Event { var out []testserver.Event for _, e := range f.srv.Events() { @@ -97,6 +100,7 @@ func (f *fixture) downloads() []testserver.Event { out = append(out, e) } } + sort.Slice(out, func(i, j int) bool { return out[i].Start < out[j].Start }) return out } From 9cccecf7d7f29bccf05e1eb3ada8accf3098cfe4 Mon Sep 17 00:00:00 2001 From: selul Date: Thu, 5 Nov 2026 12:17:33 +0200 Subject: [PATCH 11/11] url: repeat only a plain mistyped flag name in flag errors A pasted argument can hold spaces or newlines ("--api-key KEY" as one argument, a key on a second line). Flag errors now drop the whole argument of bad flag syntax, and name an unknown flag only when it looks like one (letters, digits and dashes, under 20 characters). export tests: the stalled-download test waits until the fake has recorded both requests, since the stalled one is recorded only when its handler sees the cancel. Co-Authored-By: Claude Opus 5.5 --- internal/cli/url.go | 18 +++++++++++------- internal/cli/url_test.go | 4 +++- internal/export/export_test.go | 8 +++++++- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/internal/cli/url.go b/internal/cli/url.go index 86e348b..ed4291d 100644 --- a/internal/cli/url.go +++ b/internal/cli/url.go @@ -254,23 +254,27 @@ func runURL(ctx context.Context, args []string, env Env) int { // (`invalid value "x" for flag -c`, `invalid boolean value "x" for -json`), // a whole argument (`bad flag syntax: ---x`) and a flag name // (`flag provided but not defined: -x`). +// The arguments, which can span lines, run to the end of the message. var ( flagValue = regexp.MustCompile(`(invalid (?:boolean )?value) "(?:[^"\\]|\\.)*"`) - flagSyntax = regexp.MustCompile(`(bad flag syntax): .*`) - flagUnknown = regexp.MustCompile(`(flag provided but not defined: -)(\S+)`) + flagSyntax = regexp.MustCompile(`(?s)(bad flag syntax): .*`) + flagUnknown = regexp.MustCompile(`(?s)flag provided but not defined: -(.*)`) + // flagName is what a mistyped flag looks like; anything else (a value + // pasted with it, a key) is not repeated. + flagName = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9-]{0,18}$`) ) // flagErrorWriter drops from the flag package's errors the values and -// arguments typed, and a flag name that looks like an API key: one could be -// a key given by mistake, from any source. +// arguments typed, keeping only a plain mistyped flag name: what was typed +// could be an API key given by mistake, from any source. type flagErrorWriter struct{ w io.Writer } func (f flagErrorWriter) Write(p []byte) (int, error) { s := flagValue.ReplaceAllString(string(p), "$1") - s = flagSyntax.ReplaceAllString(s, "$1") + s = flagSyntax.ReplaceAllString(s, "$1\n") s = flagUnknown.ReplaceAllStringFunc(s, func(m string) string { - if name := flagUnknown.FindStringSubmatch(m)[2]; keyLike.MatchString(name) { - return "flag provided but not defined (it looks like an API key, so it is not shown)" + if name := strings.TrimSuffix(flagUnknown.FindStringSubmatch(m)[1], "\n"); !flagName.MatchString(name) { + return "flag provided but not defined (what was typed is not shown)\n" } return m }) diff --git a/internal/cli/url_test.go b/internal/cli/url_test.go index 6177b08..33df266 100644 --- a/internal/cli/url_test.go +++ b/internal/cli/url_test.go @@ -471,7 +471,9 @@ func TestURLRedactsKeysInEveryMessage(t *testing.T) { {[]string{"--width", "--", "--api-key", other, "-c", other}, "invalid value for flag -c"}, {[]string{"--api-key", other, "-c", secret, "--api-key", "x"}, "invalid value for flag -c"}, {[]string{"---api-key=" + secret}, "bad flag syntax"}, - {[]string{"--optml-s" + strings.Repeat("Ab3", 19)}, "it looks like an API key"}, + {[]string{"--optml-s" + strings.Repeat("Ab3", 19)}, "what was typed is not shown"}, + {[]string{"---api-key=\n" + secret}, "bad flag syntax\n"}, + {[]string{"--api-key " + secret}, "what was typed is not shown"}, {[]string{"--widht", "300"}, "flag provided but not defined: -widht"}, } { if code := h.run(append([]string{"url", urlImage}, tt.args...)...); code != ExitUsage { diff --git a/internal/export/export_test.go b/internal/export/export_test.go index 86467c8..68d82bd 100644 --- a/internal/export/export_test.go +++ b/internal/export/export_test.go @@ -361,7 +361,13 @@ func TestRunRestartsAStalledDownload(t *testing.T) { if out.Err != nil || out.Downloaded != 1 { t.Fatalf("outcome: %+v", out) } - if d := f.downloads(); len(d) != 2 || d[1].Range != "bytes=60-" { + // The stalled request is recorded once its handler sees the cancel, + // which can be after the run returns. + d := f.downloads() + for deadline := time.Now().Add(2 * time.Second); len(d) < 2 && time.Now().Before(deadline); d = f.downloads() { + time.Sleep(5 * time.Millisecond) + } + if len(d) != 2 || d[1].Range != "bytes=60-" { t.Errorf("downloads: %+v", d) } }