From bf4b1ab78074e900abc18018077dae99899f9aaf Mon Sep 17 00:00:00 2001 From: ysyneu Date: Thu, 5 Nov 2026 01:02:39 -0800 Subject: [PATCH 1/2] feat(artifacts): stream file downloads; no overall timeout on byte transfers Artifacts.ReadStream went through the typed JSON path, which is wrong for a file of arbitrary type and size: - a JSON file was decoded as the response envelope, so Raw came back empty (or, if it looked like an error envelope, as an API error); - Raw is capped at 10MB, so larger files were silently truncated; - http.Client.Timeout (30s by default) also bounds reading the body. The generator now skips artifact-read-stream, and the hand-written Artifacts.Download(ctx, token) returns the live body for the caller to copy. Byte transfers (file upload, file download, session export, session subscribe) now share one transferClient: the configured client without an overall timeout, so a long but healthy transfer is not cut off. Callers bound them with ctx. JSON calls keep the client timeout. --- artifacts.go | 11 ----- artifacts_download.go | 26 +++++++++++ artifacts_download_test.go | 88 ++++++++++++++++++++++++++++++++++++++ flashduty.go | 11 +++++ internal/cmd/gen/main.go | 7 +++ models_gen.go | 8 ---- sessions_export.go | 18 ++++---- sessions_subscribe.go | 12 ++---- upload.go | 2 +- 9 files changed, 147 insertions(+), 36 deletions(-) create mode 100644 artifacts_download.go create mode 100644 artifacts_download_test.go diff --git a/artifacts.go b/artifacts.go index f72c9ef..c5e978d 100644 --- a/artifacts.go +++ b/artifacts.go @@ -63,17 +63,6 @@ func (s *ArtifactsService) ReadSign(ctx context.Context, req *ArtifactSignReques return out, resp, nil } -// Download or preview a file. -// -// Download or preview a file's bytes using a signed token. -// -// The success body is a file, not a JSON envelope: it is returned on Response.Raw. -// -// API: GET /safari/artifact/stream (artifact-read-stream). -func (s *ArtifactsService) ReadStream(ctx context.Context, req *ArtifactsReadStreamRequest) (*Response, error) { - return s.client.doGet(ctx, "/safari/artifact/stream", req, nil) -} - // Remove artifact from gallery. // // Detach an artifact from the gallery; the source file stays with its session. diff --git a/artifacts_download.go b/artifacts_download.go new file mode 100644 index 0000000..3a805a8 --- /dev/null +++ b/artifacts_download.go @@ -0,0 +1,26 @@ +package flashduty + +import ( + "context" + "io" + "net/http" + "net/url" +) + +// Download streams the bytes of a presented file, using a token from +// ArtifactsService.ReadSign (the `t` query parameter of its download_url). +// +// The body is the file itself — any content type, any size — so it is returned +// as the live HTTP response body, never decoded or buffered. The caller owns +// the io.ReadCloser and MUST Close it. The request has no client-side overall +// timeout; bound it with ctx. resp.Header carries the file's Content-Type, +// Content-Length and Content-Disposition (with the file name). +// +// On a non-2xx status the JSON error envelope is surfaced as a typed error +// (*ErrorResponse, or *RateLimitError on 429) with a nil ReadCloser. +// +// API: GET /safari/artifact/stream (artifact-read-stream), mode=download. +func (s *ArtifactsService) Download(ctx context.Context, token string) (io.ReadCloser, *Response, error) { + q := url.Values{"t": {token}, "mode": {"download"}} + return s.client.openStream(ctx, http.MethodGet, "/safari/artifact/stream?"+q.Encode(), "*/*", nil) +} diff --git a/artifacts_download_test.go b/artifacts_download_test.go new file mode 100644 index 0000000..0b02d21 --- /dev/null +++ b/artifacts_download_test.go @@ -0,0 +1,88 @@ +package flashduty + +import ( + "bytes" + "context" + "errors" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +func TestArtifactDownloadReturnsFileBytesVerbatim(t *testing.T) { + // A JSON file shaped like an error envelope: the typed path would decode it + // as an API error; Download must hand it back untouched. + file := []byte(`{"error":{"code":"InvalidParameter","message":"not an API error"}}`) + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet || r.URL.Path != "/safari/artifact/stream" { + t.Errorf("request = %s %s", r.Method, r.URL.Path) + } + q := r.URL.Query() + if q.Get("t") != "TOK" || q.Get("mode") != "download" || q.Get("app_key") != "KEY" { + t.Errorf("query = %s", r.URL.RawQuery) + } + w.Header().Set("Content-Type", "application/json") + w.Header().Set("Content-Disposition", `attachment; filename="data.json"`) + _, _ = w.Write(file) + }) + + rc, resp, err := c.Artifacts.Download(context.Background(), "TOK") + if err != nil { + t.Fatalf("Download: %v", err) + } + defer func() { _ = rc.Close() }() + got, _ := io.ReadAll(rc) + if !bytes.Equal(got, file) { + t.Fatalf("body = %q, want %q", got, file) + } + if cd := resp.Header.Get("Content-Disposition"); cd != `attachment; filename="data.json"` { + t.Fatalf("Content-Disposition = %q", cd) + } +} + +func TestArtifactDownloadIsNotCappedBySizeOrClientTimeout(t *testing.T) { + big := bytes.Repeat([]byte("x"), maxResponseBodySize+1024) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write(big[:1024]) + w.(http.Flusher).Flush() + time.Sleep(150 * time.Millisecond) // longer than the client Timeout below + _, _ = w.Write(big[1024:]) + })) + t.Cleanup(srv.Close) + c, err := NewClient("KEY", WithBaseURL(srv.URL), WithLogger(noopLogger{}), WithTimeout(50*time.Millisecond)) + if err != nil { + t.Fatal(err) + } + + rc, _, err := c.Artifacts.Download(context.Background(), "TOK") + if err != nil { + t.Fatalf("Download: %v", err) + } + defer func() { _ = rc.Close() }() + n, err := io.Copy(io.Discard, rc) + if err != nil { + t.Fatalf("read body: %v", err) + } + if n != int64(len(big)) { + t.Fatalf("read %d bytes, want %d", n, len(big)) + } +} + +func TestArtifactDownloadSurfacesErrorEnvelope(t *testing.T) { + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusGone) + _, _ = io.WriteString(w, `{"request_id":"RID","error":{"code":"ResourceNotFound","message":"runner offline"}}`) + }) + + rc, _, err := c.Artifacts.Download(context.Background(), "TOK") + if rc != nil { + t.Fatal("want nil ReadCloser on error") + } + var apiErr *ErrorResponse + if !errors.As(err, &apiErr) || apiErr.Message != "runner offline" || apiErr.RequestID != "RID" { + t.Fatalf("err = %#v", err) + } +} diff --git a/flashduty.go b/flashduty.go index 146b149..ecf9046 100644 --- a/flashduty.go +++ b/flashduty.go @@ -227,6 +227,17 @@ func (c *Client) doMethodWithAppKey(ctx context.Context, method, path string, bo return c.processResponse(httpResp, out) } +// transferClient returns the client for byte transfers (file uploads, file +// downloads, export streams). It shares c.client's Transport and settings but +// has no overall Timeout: http.Client.Timeout also bounds reading the body, so +// the 30s default that suits JSON calls cuts off any transfer that takes +// longer, however healthy. The caller bounds a transfer through ctx instead. +func (c *Client) transferClient() *http.Client { + hc := *c.client + hc.Timeout = 0 + return &hc +} + // processResponse drains and interprets an HTTP response: it unwraps the // {request_id, error, data} envelope, decodes data into out (when non-nil), // and surfaces non-JSON success bodies on Response.Raw. Hand-written methods diff --git a/internal/cmd/gen/main.go b/internal/cmd/gen/main.go index 4bc08d2..d34791f 100644 --- a/internal/cmd/gen/main.go +++ b/internal/cmd/gen/main.go @@ -264,6 +264,13 @@ func (g *Gen) collectServices(paths map[string]any) []service { // Response.Raw, and they generate as ordinary raw methods returning (*Response, // error). func isStreamingOp(o map[string]any) bool { + // The artifact stream returns a user's file verbatim: any content type (a + // JSON file would be decoded as the envelope and lost) and any size (Raw is + // capped at maxResponseBodySize). It gets a hand-written streaming method + // (see artifacts_download.go) even though the spec declares octet-stream. + if str(o, "operationId") == "artifact-read-stream" { + return true + } resp := asMap(asMap(o["responses"])["200"]) content := asMap(resp["content"]) if len(content) == 0 { diff --git a/models_gen.go b/models_gen.go index 5aec410..1043a7b 100644 --- a/models_gen.go +++ b/models_gen.go @@ -14329,14 +14329,6 @@ type PreviewTemplateResponseCardElementsItemActionsItem struct { Text string `json:"text" toon:"text"` } -// ArtifactsReadStreamRequest holds the query parameters for Download or preview a file. -type ArtifactsReadStreamRequest struct { - // Signed token issued by `POST /safari/artifact/sign`. Bound to the calling account and person; valid for 5 minutes. - T string `url:"t"` - // `download` (default) serves the file as an attachment; `preview` serves it inline for browser display. Any other value falls back to `download`. - Mode string `url:"mode,omitempty"` -} - // IncidentsPostMortemInfoRequest holds the query parameters for Get post-mortem. type IncidentsPostMortemInfoRequest struct { // Post-mortem ID: a 32-character hex string derived from the account ID and the linked incident IDs. diff --git a/sessions_export.go b/sessions_export.go index 7fe1a3b..a71d903 100644 --- a/sessions_export.go +++ b/sessions_export.go @@ -28,21 +28,23 @@ import ( // API: POST /safari/session/export (session-read-export). func (s *SessionsService) Export(ctx context.Context, req *SessionExportRequest) (io.ReadCloser, *Response, error) { // The success body is a stream, not a JSON envelope; ask for NDJSON. - return s.client.openStream(ctx, s.client.client, "/safari/session/export", "application/x-ndjson", req) + return s.client.openStream(ctx, http.MethodPost, "/safari/session/export", "application/x-ndjson", req) } -// openStream POSTs req and returns the live success body for the caller to -// close. Error responses are JSON envelopes even on a streaming endpoint: the -// (bounded) body is drained and closed and surfaced as a typed error. hc is the -// http.Client to send with, so a long-lived stream can drop the overall timeout. -func (c *Client) openStream(ctx context.Context, hc *http.Client, path, accept string, req any) (io.ReadCloser, *Response, error) { - httpReq, err := c.newRequest(ctx, http.MethodPost, path, req) +// openStream sends a request (POST with a JSON body, or GET with path already +// carrying its query) on the transfer client and returns the live success body +// for the caller to close. The transfer client has no overall timeout, so ctx is +// what bounds the stream. Error responses are JSON envelopes even on a streaming +// endpoint: the (bounded) body is drained and closed and surfaced as a typed +// error. +func (c *Client) openStream(ctx context.Context, method, path, accept string, req any) (io.ReadCloser, *Response, error) { + httpReq, err := c.newRequest(ctx, method, path, req) if err != nil { return nil, nil, err } httpReq.Header.Set("Accept", accept) - httpResp, err := hc.Do(httpReq) + httpResp, err := c.transferClient().Do(httpReq) if err != nil { return nil, nil, fmt.Errorf("flashduty: request to %s failed: %v", sanitizeURL(httpReq.URL), sanitizeError(err)) } diff --git a/sessions_subscribe.go b/sessions_subscribe.go index 15de917..b4522fe 100644 --- a/sessions_subscribe.go +++ b/sessions_subscribe.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "io" + "net/http" "strings" ) @@ -41,20 +42,15 @@ type SessionEventFrame struct { // Subscribe opens POST /safari/session/subscribe and returns the live SSE // response body. The caller owns it and MUST Close it; read frames with // NewSessionFrameReader. The stream is unbounded in time (the server closes it -// after an idle period), so it is served by a client with no overall timeout: -// bound its lifetime with ctx. +// after an idle period); like every stream it is served by the transfer client +// with no overall timeout, so bound its lifetime with ctx. // // On any non-2xx status the body is the usual JSON error envelope: Subscribe // reads and closes it and returns a typed error with a nil ReadCloser. // // API: POST /safari/session/subscribe. func (s *SessionsService) Subscribe(ctx context.Context, req *SessionSubscribeRequest) (io.ReadCloser, *Response, error) { - // The shared client's Timeout also bounds reading the body, which would cut - // a long-lived stream. Copy it without the overall timeout; the transport - // and redirect policy are shared. - streamClient := *s.client.client - streamClient.Timeout = 0 - return s.client.openStream(ctx, &streamClient, "/safari/session/subscribe", "text/event-stream", req) + return s.client.openStream(ctx, http.MethodPost, "/safari/session/subscribe", "text/event-stream", req) } // SessionStreamError is a server-reported error carried in-band on the stream diff --git a/upload.go b/upload.go index 24167c1..50b78d8 100644 --- a/upload.go +++ b/upload.go @@ -90,7 +90,7 @@ func (c *Client) uploadFile(ctx context.Context, path string, query url.Values, "body", "", ) - httpResp, err := c.client.Do(req) + httpResp, err := c.transferClient().Do(req) if err != nil { return nil, fmt.Errorf("flashduty: request to %s failed: %v", sanitizeURL(req.URL), sanitizeError(err)) } From 5be8d556bcd3ef2fdb3baeabe55de012666f801d Mon Sep 17 00:00:00 2001 From: ysyneu Date: Thu, 5 Nov 2026 01:09:52 -0800 Subject: [PATCH 2/2] fix: redact the signed download token in logged URLs; document timeout scope sanitizeURL only redacted app_key, so the t token of an artifact download (valid 5 minutes) reached request logs. WithTimeout/WithHTTPClient now state that their timeout bounds JSON calls only; byte transfers are bounded by ctx. --- options.go | 8 ++++++-- redact.go | 11 +++++++++-- redact_test.go | 11 +++++++++++ 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/options.go b/options.go index 6b576c6..7cbfeae 100644 --- a/options.go +++ b/options.go @@ -30,7 +30,9 @@ func WithBaseURL(raw string) Option { } } -// WithTimeout sets the HTTP client timeout. +// WithTimeout sets the overall timeout of JSON calls. Byte transfers (file +// upload and download, session export and subscribe) ignore it — an overall +// timeout also bounds reading the body — and are bounded by their ctx instead. func WithTimeout(d time.Duration) Option { return func(c *Client) { c.client.Timeout = d } } @@ -40,7 +42,9 @@ func WithUserAgent(ua string) Option { return func(c *Client) { c.UserAgent = ua } } -// WithHTTPClient supplies a custom *http.Client. Nil is ignored. +// WithHTTPClient supplies a custom *http.Client. Nil is ignored. Its Timeout +// applies to JSON calls only; byte transfers use a copy without it (see +// WithTimeout). func WithHTTPClient(hc *http.Client) Option { return func(c *Client) { if hc != nil { diff --git a/redact.go b/redact.go index ba817c0..fa09319 100644 --- a/redact.go +++ b/redact.go @@ -18,8 +18,15 @@ const ( func sanitizeURL(u *url.URL) string { sanitized := *u q := sanitized.Query() - if q.Has("app_key") { - q.Set("app_key", "[REDACTED]") + redacted := false + // app_key is the credential; t is a signed download token (artifact stream). + for _, k := range []string{"app_key", "t"} { + if q.Has(k) { + q.Set(k, "[REDACTED]") + redacted = true + } + } + if redacted { sanitized.RawQuery = q.Encode() } return sanitized.String() diff --git a/redact_test.go b/redact_test.go index 2203666..db1b491 100644 --- a/redact_test.go +++ b/redact_test.go @@ -19,6 +19,17 @@ func TestSanitizeURLRedactsAppKey(t *testing.T) { } } +func TestSanitizeURLRedactsDownloadToken(t *testing.T) { + u, _ := url.Parse("https://api.flashcat.cloud/safari/artifact/stream?app_key=KEY&mode=download&t=SIGNED") + got := sanitizeURL(u) + if strings.Contains(got, "SIGNED") || strings.Contains(got, "KEY") { + t.Fatalf("credential leaked: %s", got) + } + if !strings.Contains(got, "mode=download") { + t.Fatalf("non-secret parameter lost: %s", got) + } +} + func TestSanitizeBodyRedactsSecrets(t *testing.T) { in := `{"name":"n","password":"hunter2","env":{"OPENAI_API_KEY":"sk-xyz"}}` got := sanitizeBody(in)