diff --git a/docs/knowledge/code/internal-platform-meta.md b/docs/knowledge/code/internal-platform-meta.md index c1244660..3d9158c7 100644 --- a/docs/knowledge/code/internal-platform-meta.md +++ b/docs/knowledge/code/internal-platform-meta.md @@ -89,5 +89,16 @@ past start date is refused, with a same-day ad-set `start_time` nudged to now+buffer. `doRequest` retries HTTP 429 and Graph rate-limit envelope codes (4/17/32/341/613/80004) with bounded backoff, draining the body before close, and a truncated response body is surfaced rather than reported as a false success. +Redirect following is force-disabled (a shared `noFollow` `CheckRedirect` policy). +For a `WithHTTPClient`-supplied client, `NewClient` builds a FRESH `*http.Client` +carrying the caller's reusable exported fields (`Transport`, `Jar`, `Timeout`) with +`CheckRedirect: noFollow`, rather than value-copying the caller's client (an +`http.Client` must not be copied after first use — a copy duplicates its internal +mutex while sharing the request-cancellation map). So a 3xx on a mutating POST is +surfaced rather than followed to a different target; `createOutcomeAmbiguous` +classifies a mutating 3xx (gated on the method) and any 5xx/transport failure as +UNCONFIRMED, so a create that may have committed is not blind-retried into a +duplicate. The status is preserved even when the response body is unreadable or +oversized, so an ambiguous outcome is never downgraded to a definite failure. See [internal/platform/meta](../../../internal/platform/meta). diff --git a/docs/knowledge/code/internal-platform-twitter.md b/docs/knowledge/code/internal-platform-twitter.md index 80222e8a..a3816517 100644 --- a/docs/knowledge/code/internal-platform-twitter.md +++ b/docs/knowledge/code/internal-platform-twitter.md @@ -47,6 +47,12 @@ toward the 1-req/sec limit; it does NOT enforce the account-wide write limit across concurrent dispatches or replicas (that needs cross-replica coordination, tracked in LFXV2-2665), so operators must not rely on this stateless client for cross-dispatch rate limiting. When the account limit is hit anyway, 429s are -retried with backoff bounded by `Retry-After` / `X-Rate-Limit-Reset`. +retried with backoff bounded by `Retry-After` / `X-Rate-Limit-Reset`. Redirect +following is force-disabled (a shared `noFollow` `CheckRedirect` policy on the +default and any `WithHTTPClient`-supplied client, via a shallow copy) so a 3xx is +surfaced rather than followed — important with OAuth 1.0a, where a followed +redirect would resend a request signed for the original URL to a different one. +(Typing the non-2xx error and classifying a mutating 3xx as UNCONFIRMED, as the +meta/reddit clients do, is tracked as a follow-up — LFXV2-2642.) See [internal/platform/twitter](../../../internal/platform/twitter). diff --git a/docs/knowledge/log.md b/docs/knowledge/log.md index 237545de..d6c8a4b4 100644 --- a/docs/knowledge/log.md +++ b/docs/knowledge/log.md @@ -1,5 +1,58 @@ # Log +## 2026-07-20 + +**Update** — Extended the Meta ad-set ambiguity to the 2xx-no-id case (LFXV2-2641, +PR #30 review by Copilot). The ad-set create's error path already routed through +`createOutcomeAmbiguous`, but a 2xx response with an empty `id` fell through to a +definite "returned no ad set ID" — the same duplicate-create risk as the campaign +and twitter no-id paths. Now surfaces UNCONFIRMED (verify before retrying). Test +added. Also fixed a CI `check-fmt` failure (gofmt comment alignment in the meta +test). + +## 2026-07-19 + +**Update** — Fixed an http.Client copy-after-use in the Meta client's no-follow +enforcement (LFXV2-2641, PR #30 review by Copilot). `NewClient` value-copied a +`WithHTTPClient`-supplied client (`hc := *c.httpClient`) to override CheckRedirect +— but an `http.Client` must not be copied after first use (the copy duplicates its +internal mutex while sharing the request-cancellation map, so concurrent use of +the caller's client and the copy can race). Now builds a FRESH `*http.Client` +carrying only the exported reusable fields (Transport, Jar, Timeout) with +`CheckRedirect: noFollow`. The no-follow test asserts Transport/Timeout are +preserved and the fresh client is a distinct pointer. Also made the campaign +UNCONFIRMED step reason-neutral ("ambiguous response — timeout, server error, or +an unfollowed redirect") since a 3xx now routes there too. NOTE: the reddit client +(merged) has the same value-copy pattern — follow-up tracked to apply the same +fresh-client fix there. The twitter client gets the same fix on PR #31. + +**Update** — Closed two more Meta ambiguity gaps (LFXV2-2641, PR #30 review by +Copilot). (1) `doRequest` returned a plain error when a NON-2xx response body +failed to read, stripping the HTTP status — so a mutating 3xx/5xx with an +unreadable body (the create may have committed) was mis-seen as a definite failure +by `createOutcomeAmbiguous` (which keys on the `*APIError` status). It now returns +an `*APIError` preserving the status on a non-2xx read failure (2xx read failures +stay `transportError`). (2) The ad-set create returned its error directly without +the ambiguity check the campaign and ad/creative creates use, so a surfaced 3xx/5xx +read as a definite "ad set creation failed" — risking a duplicate ad set on retry. +It now routes through `createOutcomeAmbiguous`: ambiguous → UNCONFIRMED (verify +before retrying), definite 4xx → "failed". Tests added for both. (3) The same +status-stripping existed in the OVERSIZED-body branch (> maxResponseBody, 10 MiB), which returned a +plain error before recording the status — a mutating 3xx/5xx over the cap was still +mis-classified as a definite failure. Now the oversized-body branch preserves the +status the same way (2xx → transportError, non-2xx → *APIError), with a regression +test. Updated the meta concept doc to describe the fresh-client + status-preservation. + +**Update** — Gated the Meta client's 3xx create-outcome ambiguity on a mutating +method (LFXV2-2641, PR #30 review by Cursor Bugbot). `createOutcomeAmbiguous` +treated EVERY 3xx as UNCONFIRMED without checking the method, diverging from the +reddit client (which gates 3xx on `isMutatingMethod`) despite claiming to mirror +it. All call sites pass POST today so behavior was unchanged, but the helper's +contract was wrong for any future GET caller — a GET redirect is not a create. +Added `isMutatingMethod` to the meta client and gated the 3xx branch (5xx and +transport errors stay ambiguous regardless of method); extended the ambiguity test +with GET/POST/DELETE method cases. Now genuinely identical to reddit. + ## 2026-07-18 **Creation** — Added the `internal/platform/googleads` Go package (GA-1 scaffold, @@ -13,6 +66,23 @@ campaign.start_date_time/end_date_time. Concept doc + code index updated. Campai creation (:mutate), metrics/keywords/audience, and keyword actions follow in GA-2..GA-5. +**Update** — Also strengthened the no-follow regression tests (meta + twitter): +they injected a nil-`CheckRedirect` client, which couldn't prove the override is +UNCONDITIONAL (a "fill only nil callbacks" impl would pass). Now they inject a +caller client carrying a SENTINEL `CheckRedirect` and assert the client the code +uses returns `http.ErrUseLastResponse` despite it, while the caller's original +still returns the sentinel (shallow copy, not mutation). (PR #30 review by Copilot.) + +**Update** — Disabled HTTP redirect following on the Meta and X/Twitter Ads +clients (LFXV2-2641), closing a duplicate-create gap: both built their +`*http.Client` (and accepted `WithHTTPClient` clients) with no `CheckRedirect`, so +the stdlib could follow a 3xx on a mutating POST after the create was committed and +muddy outcome classification (for X, a followed redirect also resends an OAuth-1.0a +request signed for the original URL). Added a shared `noFollow` +(`http.ErrUseLastResponse`) policy set on the default client and enforced +unconditionally after options via a shallow copy (so a caller's client isn't +mutated) — matching the reddit/linkedin/googleads clients. Regression tests added. + ## 2026-07-15 **Update** — Hardened the Reddit Ads client's ambiguous-outcome classification diff --git a/internal/platform/meta/client.go b/internal/platform/meta/client.go index c44f1b9b..e28ca4b4 100644 --- a/internal/platform/meta/client.go +++ b/internal/platform/meta/client.go @@ -298,7 +298,21 @@ type Client struct { // Option customizes a Client. type Option func(*Client) -// WithHTTPClient overrides the HTTP client (useful for tests / timeouts). +// noFollow is the CheckRedirect policy for every client this package uses: it +// returns http.ErrUseLastResponse so the client does NOT follow redirects and +// hands the 3xx response back to the request layer, where a non-2xx status is +// surfaced as an error. The Graph API returns JSON directly and never legitimately +// 3xx-redirects these calls; not following keeps outcome classification sound — a +// redirect can't carry an already-sent mutating POST to a different target and be +// misclassified. It is shared by the built-in client and the caller-supplied- +// client enforcement in NewClient. Mirrors the reddit/linkedin/googleads clients. +func noFollow(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse +} + +// WithHTTPClient overrides the HTTP client (useful for tests / timeouts). Redirect +// following is force-disabled on whatever client ends up in use (see NewClient), +// so an injected client cannot reintroduce redirect following. func WithHTTPClient(h *http.Client) Option { return func(c *Client) { // Ignore a nil client so the safe default installed by NewClient isn't @@ -355,7 +369,7 @@ func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client c := &Client{ creds: creds, account: account, - httpClient: &http.Client{Timeout: DefaultRequestTimeout}, + httpClient: &http.Client{Timeout: DefaultRequestTimeout, CheckRedirect: noFollow}, baseURL: DefaultBaseURL, adsManagerURL: DefaultAdsManagerURL, timeNow: time.Now, @@ -364,6 +378,27 @@ func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client for _, o := range opts { o(c) } + // Enforce the no-follow redirect policy UNCONDITIONALLY on whatever client ended + // up on c.httpClient — INCLUDING one supplied via WithHTTPClient, which replaces + // the default above. Following a redirect would carry an already-sent mutating + // POST to a different target and muddy outcome classification, so no-follow is a + // correctness requirement, not a default. + // + // Build a FRESH *http.Client rather than value-copying the caller's: an + // http.Client must not be copied after first use (a value copy duplicates its + // internal mutex while sharing the request-cancellation map, so concurrent use + // of the caller's client and our copy can race). We carry over only the exported, + // reusable fields (Transport, Jar, Timeout) — the shareable connection pool / + // cookie jar / deadline — and set our own CheckRedirect. The caller's client is + // never mutated and is safe to keep using elsewhere. + if c.httpClient != nil { + c.httpClient = &http.Client{ + Transport: c.httpClient.Transport, + CheckRedirect: noFollow, + Jar: c.httpClient.Jar, + Timeout: c.httpClient.Timeout, + } + } return c } @@ -638,6 +673,10 @@ func isPreSendDialError(err error) bool { // reaches here), so the request may have been received; // - *APIError with a 5xx status: Meta received it and may have committed the // mutation before erroring. +// - *APIError with a 3xx status: redirect following is force-disabled (see +// noFollow), so a 3xx is surfaced here rather than followed. A 3xx on a +// mutating request is NOT a definite rejection — Meta may have committed the +// create and then returned a redirect — so it is ambiguous like a 5xx. // // A definite 4xx (Meta rejected it), or any pre-send failure (token missing, // body encode/build, a pre-connect dial error), means NOT applied → returns @@ -649,7 +688,30 @@ func createOutcomeAmbiguous(err error) bool { return true } var ae *APIError - return errors.As(err, &ae) && ae.StatusCode >= 500 + if !errors.As(err, &ae) { + return false + } + // A 5xx may follow a committed create. + if ae.StatusCode >= 500 { + return true + } + // A 3xx on a MUTATING request reached a responder and may have committed a + // resource before redirecting — UNCONFIRMED. A 3xx on a GET is not a create, so + // it stays non-ambiguous. Gating on the method (rather than treating every 3xx + // as ambiguous) keeps this helper's contract correct for any caller, not just + // the create path — and makes it genuinely identical to the reddit client. + return ae.StatusCode >= 300 && ae.StatusCode < 400 && isMutatingMethod(ae.Method) +} + +// isMutatingMethod reports whether an HTTP method can create/modify server state, +// so a 3xx on it may hide a committed mutation. Mirrors the reddit client. +func isMutatingMethod(method string) bool { + switch method { + case http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete: + return true + default: + return false + } } // doRequest performs a Graph API call and decodes the JSON body into out. @@ -702,14 +764,27 @@ func (c *Client) doRequest(ctx context.Context, method, path string, body map[st // returns EOF (not an error) at the limit, so an oversized body would // otherwise be silently truncated and mis-parsed as a valid short response. raw, readErr := io.ReadAll(io.LimitReader(resp.Body, maxResponseBody+1)) - if readErr == nil && int64(len(raw)) > maxResponseBody { - _ = resp.Body.Close() - return fmt.Errorf("meta API %s %s: response exceeds %d bytes", method, path, maxResponseBody) - } retryAfter := c.parseRetryAfter(resp) status := resp.StatusCode _ = resp.Body.Close() + if readErr == nil && int64(len(raw)) > maxResponseBody { + // Oversized body: we can't trust the payload, but the STATUS must still be + // preserved for the same reason as a read failure below — a mutating 3xx/5xx + // (or a 2xx) may reflect a committed create, and stripping the status would + // mis-classify it as a definite failure and invite a duplicate on retry. A + // 2xx is ambiguous (transportError); a non-2xx carries its status via + // *APIError. (An oversized error/redirect body is anomalous, but we classify + // on status, not payload.) + if status >= 200 && status < 300 { + return &transportError{Method: method, Path: path, Err: fmt.Errorf("response exceeds %d bytes", maxResponseBody)} + } + return &APIError{ + StatusCode: status, Method: method, Path: path, + Message: fmt.Sprintf("response exceeds %d bytes", maxResponseBody), + } + } + // Meta reports throttling either as HTTP 429 or, commonly, as HTTP 400 with // a Graph error envelope whose code is a known rate-limit code. Treat both // as retryable with the same bounded backoff. The envelope is only consumed @@ -731,13 +806,23 @@ func (c *Client) doRequest(ctx context.Context, method, path string, body map[st if readErr != nil && (!throttled || attempt >= retryMax) { // A read failure on a 2xx is AMBIGUOUS: Meta committed the mutation but we // couldn't read the result — wrap it as transportError so a create is - // treated as "may exist". A read failure on a non-2xx isn't a committed - // mutation, so return it plain. Mirrors the reddit client wrapping 2xx - // read/decode failures as transportError. + // treated as "may exist". Mirrors the reddit client wrapping 2xx read/decode + // failures as transportError. if status >= 200 && status < 300 { return &transportError{Method: method, Path: path, Err: fmt.Errorf("read response body: %w", readErr)} } - return fmt.Errorf("meta API %s %s: read response body: %w", method, path, readErr) + // A read failure on a NON-2xx still must preserve the HTTP status: a + // mutating 3xx (redirect, not followed) or 5xx may have committed the create + // before the unreadable body, and createOutcomeAmbiguous classifies on the + // *APIError status. Returning a plain error here would strip the status and + // silently turn an ambiguous create into a definite "failed" — the exact + // duplicate-on-retry risk the no-follow + ambiguity handling exists to close. + // The body couldn't be read, so no Graph envelope diagnostics are available; + // carry the status/method/path and note the read failure in the message. + return &APIError{ + StatusCode: status, Method: method, Path: path, + Message: fmt.Sprintf("read response body: %v", readErr), + } } if throttled && attempt < retryMax { @@ -1773,7 +1858,7 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig // (retry-safe idempotency is tracked in LFXV2-2665). Mirrors the reddit // client's createOutcomeAmbiguous handling. if createOutcomeAmbiguous(err) { - steps = append(steps, "Campaign creation outcome is UNCONFIRMED (timeout or server error); a PAUSED campaign may exist — verify by name in Meta Ads Manager") + steps = append(steps, "Campaign creation outcome is UNCONFIRMED (ambiguous response — timeout, server error, or an unfollowed redirect); a PAUSED campaign may exist — verify by name in Meta Ads Manager") return &CampaignResult{ Platform: "meta-ads", CampaignName: campaignName, @@ -1861,11 +1946,25 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig // The campaign was already created (PAUSED). Return a partial result carrying // its id so the caller can identify/clean up the orphan without parsing the // error string; auto-deleting here would race a retry that reuses it. + // + // An AMBIGUOUS ad-set failure (transport/timeout, a mutating 3xx now surfaced + // because redirects aren't followed, or a 5xx) can occur AFTER Meta committed + // the ad set — a definite "failed" instruction would let a retry create a + // DUPLICATE ad set. Word it UNCONFIRMED (verify before retrying) in that case; + // a clear 4xx rejection means nothing was created, so keep the plain "failed" + // wording. Mirrors the campaign and ad/creative create paths. + if createOutcomeAmbiguous(err) { + return partialResult(), fmt.Errorf("meta ad set creation UNCONFIRMED (campaign %s created, PAUSED; an ad set may exist — verify in Meta Ads Manager before retrying): %w", campaignID, err) + } return partialResult(), fmt.Errorf("meta ad set creation failed (campaign %s created, PAUSED): %w", campaignID, err) } adSetID = adSetResp.ID if adSetID == "" { - return partialResult(), fmt.Errorf("meta ad set creation succeeded but returned no ad set ID (campaign %s created, PAUSED)", campaignID) + // A 2xx with no id is a malformed SUCCESS: Meta may have created the ad set + // but didn't return a usable id. UNCONFIRMED (verify before retrying), NOT a + // clean failure — a blind retry could duplicate an ad set Meta already made. + // Mirrors the campaign/ad no-id and the ad-set error-path handling. + return partialResult(), fmt.Errorf("meta ad set creation UNCONFIRMED (campaign %s created, PAUSED; Meta returned a 2xx with no ad set ID — an ad set may exist; verify in Meta Ads Manager before retrying)", campaignID) } budgetLabel := "daily" if in.LifetimeBudget { diff --git a/internal/platform/meta/client_test.go b/internal/platform/meta/client_test.go index 391e2fde..ecd4d4ec 100644 --- a/internal/platform/meta/client_test.go +++ b/internal/platform/meta/client_test.go @@ -612,6 +612,102 @@ func TestCreateCampaignAdSetFailureReturnsPartialResult(t *testing.T) { } } +// TestCreateCampaignAdSetAmbiguousIsUnconfirmed verifies that an AMBIGUOUS ad-set +// failure (a 5xx — Meta may have committed the ad set) is worded UNCONFIRMED with +// verify-before-retry, NOT a definite "failed", so a caller does not blind-retry +// into a duplicate ad set. Mirrors the campaign/ad-create ambiguity handling. +func TestCreateCampaignAdSetAmbiguousIsUnconfirmed(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/act_777") && strings.Contains(r.URL.RawQuery, "account_status"): + _, _ = io.WriteString(w, `{"name":"LF Core","account_status":1,"currency":"USD"}`) + case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/campaigns"): + _, _ = io.WriteString(w, `{"id":"camp_orphan"}`) + case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/adsets"): + // 5xx — Meta may have committed the ad set before erroring. + w.WriteHeader(http.StatusInternalServerError) + _, _ = io.WriteString(w, `{"error":{"message":"upstream","type":"OAuthException","code":2}}`) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer srv.Close() + + c := NewClient( + Credentials{AccessToken: "tok-abc"}, + AccountConfig{AccountID: "act_777", PageID: "987654321", CurrencyOffset: 100}, + WithBaseURL(srv.URL), + WithClock(fixedMetaClock()), + ) + res, err := c.CreateCampaign(context.Background(), CampaignInput{ + EventName: "KubeCon", Project: "tlf", + RegistrationURL: "https://events.example.org/kubecon", Objective: "traffic", + GeoTargets: []string{"US"}, Budget: 500, StartDate: "2026-08-01", EndDate: "2026-08-31", + Variants: []AdVariant{{PrimaryText: "Join us", Headline: "KubeCon 2026"}}, + }) + if err == nil { + t.Fatal("expected an error when ad set creation returns 5xx") + } + if res == nil || res.CampaignID != "camp_orphan" { + t.Fatalf("expected a partial result carrying the orphaned campaign id, got %+v", res) + } + // The error must convey UNCONFIRMED, not a definite "failed". + if !strings.Contains(err.Error(), "UNCONFIRMED") { + t.Errorf("ambiguous ad-set 5xx must be UNCONFIRMED, got: %v", err) + } + if strings.Contains(err.Error(), "ad set creation failed") { + t.Errorf("ambiguous ad-set 5xx must not read as a definite failure: %v", err) + } +} + +// TestCreateCampaignAdSetNoIDIsUnconfirmed verifies a 2xx ad-set create with no id +// is UNCONFIRMED (Meta may have created it), not a definite "returned no ad set ID" +// — same duplicate-avoidance as the campaign/ad and twitter no-id paths. +func TestCreateCampaignAdSetNoIDIsUnconfirmed(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/act_777") && strings.Contains(r.URL.RawQuery, "account_status"): + _, _ = io.WriteString(w, `{"name":"LF Core","account_status":1,"currency":"USD"}`) + case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/campaigns"): + _, _ = io.WriteString(w, `{"id":"camp_orphan"}`) + case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/adsets"): + _, _ = io.WriteString(w, `{}`) // 2xx, no id + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer srv.Close() + + c := NewClient( + Credentials{AccessToken: "tok-abc"}, + AccountConfig{AccountID: "act_777", PageID: "987654321", CurrencyOffset: 100}, + WithBaseURL(srv.URL), + WithClock(fixedMetaClock()), + ) + res, err := c.CreateCampaign(context.Background(), CampaignInput{ + EventName: "KubeCon", Project: "tlf", + RegistrationURL: "https://events.example.org/kubecon", Objective: "traffic", + GeoTargets: []string{"US"}, Budget: 500, StartDate: "2026-08-01", EndDate: "2026-08-31", + Variants: []AdVariant{{PrimaryText: "Join us", Headline: "KubeCon 2026"}}, + }) + if err == nil { + t.Fatal("expected an error when the ad set returns a 2xx with no id") + } + if res == nil || res.CampaignID != "camp_orphan" { + t.Fatalf("expected a partial result carrying the orphaned campaign id, got %+v", res) + } + if !strings.Contains(err.Error(), "UNCONFIRMED") { + t.Errorf("2xx ad-set with no id must be UNCONFIRMED, got: %v", err) + } + if strings.Contains(err.Error(), "returned no ad set ID") && !strings.Contains(err.Error(), "UNCONFIRMED") { + t.Errorf("2xx ad-set with no id must not read as a definite failure: %v", err) + } +} + // TestCreateCampaignNoIDReturnsPartial verifies a 2xx campaign create with no id // returns a partial result carrying the campaign name (reconcilable by name), not // a bare (nil, err). @@ -2977,6 +3073,40 @@ func TestValidateGeoTargetsDeduplicates(t *testing.T) { } } +// TestDoRequestOversizedNon2xxPreservesStatus verifies that an OVERSIZED non-2xx +// body still surfaces a typed *APIError carrying the status. Like a read failure, +// an oversized-body branch that stripped the status would mis-classify a mutating +// 3xx/5xx (create may have committed) as a definite failure. +func TestDoRequestOversizedNon2xxPreservesStatus(t *testing.T) { + // Build the payload from maxResponseBody so it actually crosses the configured + // cap (10 MiB today) and exercises the oversized-body branch — a fixed ~1 MiB + // pad would take the ordinary path and pass even if that branch were broken. + pad := strings.Repeat("x", maxResponseBody+1024) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) // 5xx — create may have committed + _, _ = io.WriteString(w, `{"pad":"`+pad+`"}`) + })) + defer srv.Close() + c := NewClient(Credentials{AccessToken: "t"}, AccountConfig{AccountID: "act_1"}, + WithBaseURL(srv.URL), withRetryBaseDelay(time.Millisecond)) + var out createResponse + err := c.doRequest(context.Background(), http.MethodPost, "/x", map[string]any{"a": 1}, &out) + if err == nil { + t.Fatal("expected an error for an oversized body, got nil") + } + var ae *APIError + if !errors.As(err, &ae) { + t.Fatalf("want *APIError preserving the status, got %T: %v", err, err) + } + if ae.StatusCode != http.StatusInternalServerError { + t.Errorf("APIError.StatusCode = %d, want 500 (status must survive an oversized body)", ae.StatusCode) + } + if !createOutcomeAmbiguous(err) { + t.Error("a mutating 500 with an oversized body must be classified ambiguous") + } +} + // TestDoRequestPropagatesBodyReadError verifies a truncated response (declared // Content-Length larger than the body sent) is reported as an error, not a // false success, even if the partial body would parse. @@ -3009,6 +3139,48 @@ func TestDoRequestPropagatesBodyReadError(t *testing.T) { } } +// TestDoRequestNon2xxReadErrorPreservesStatus verifies that when a NON-2xx +// response body fails to read, doRequest still surfaces a typed *APIError carrying +// the HTTP status (not a plain error). This matters for a mutating 3xx (surfaced +// because redirects aren't followed): the create may have committed, and +// createOutcomeAmbiguous classifies on the *APIError status — stripping it would +// silently turn an ambiguous create into a definite "failed". +func TestDoRequestNon2xxReadErrorPreservesStatus(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("Content-Length", "1000") // advertise more than we send + w.WriteHeader(http.StatusFound) // 302 — a mutating redirect, not followed + _, _ = io.WriteString(w, `{"partial":`) + if f, ok := w.(http.Flusher); ok { + f.Flush() + } + if hj, ok := w.(http.Hijacker); ok { + if conn, _, err := hj.Hijack(); err == nil { + _ = conn.Close() + } + } + })) + defer srv.Close() + c := NewClient(Credentials{AccessToken: "t"}, AccountConfig{AccountID: "act_1"}, + WithBaseURL(srv.URL), withRetryBaseDelay(time.Millisecond)) + var out createResponse + err := c.doRequest(context.Background(), http.MethodPost, "/x", map[string]any{"a": 1}, &out) + if err == nil { + t.Fatal("expected a read error, got nil") + } + var ae *APIError + if !errors.As(err, &ae) { + t.Fatalf("want *APIError preserving the status, got %T: %v", err, err) + } + if ae.StatusCode != http.StatusFound { + t.Errorf("APIError.StatusCode = %d, want 302 (status must survive a body-read failure)", ae.StatusCode) + } + // The whole point: a mutating 3xx with an unreadable body stays AMBIGUOUS. + if !createOutcomeAmbiguous(err) { + t.Error("a mutating 302 with an unreadable body must be classified ambiguous") + } +} + // TestWithHTTPClientNilIsIgnored verifies a nil client doesn't clobber the // default (which would panic on the next request). func TestWithHTTPClientNilIsIgnored(t *testing.T) { @@ -3410,3 +3582,102 @@ func TestBuildPlacementTargetingRejectsMessengerInbox(t *testing.T) { t.Errorf("error = %v, want it to name messengerInbox", err) } } + +// TestNoFollowRedirectPolicy verifies the client force-disables redirect +// following: the default client gets CheckRedirect=noFollow, and a WithHTTPClient- +// supplied client's policy is overridden by building a FRESH client (an +// http.Client must not be copied after first use), preserving the caller's +// reusable Transport/Timeout without mutating the caller's client. Following a 3xx +// on a mutating POST could carry an already-sent create to a different target. +func TestNoFollowRedirectPolicy(t *testing.T) { + // Default client. + c := NewClient(Credentials{AccessToken: "t"}, AccountConfig{AccountID: "act_1", PageID: "p1"}) + if c.httpClient.CheckRedirect == nil { + t.Fatal("default client has no CheckRedirect — redirects would be followed") + } + if err := c.httpClient.CheckRedirect(nil, nil); err != http.ErrUseLastResponse { + t.Errorf("CheckRedirect = %v, want http.ErrUseLastResponse (no-follow)", err) + } + + // Inject a client that ALREADY carries a caller-supplied redirect policy (a + // sentinel) plus a distinctive Transport and Timeout. This proves (a) the + // override is unconditional (a "fill only nil callbacks" impl would preserve the + // sentinel and re-enable following), (b) the reusable Transport/Timeout are + // carried onto the fresh client, and (c) the caller's client is not mutated and + // is NOT the same pointer (no value-copy of an http.Client after first use). + sentinel := errors.New("caller-sentinel-redirect-policy") + callerTransport := &http.Transport{} + caller := &http.Client{ + CheckRedirect: func(*http.Request, []*http.Request) error { return sentinel }, + Transport: callerTransport, + Timeout: 17 * time.Second, + } + c2 := NewClient(Credentials{AccessToken: "t"}, AccountConfig{AccountID: "act_1", PageID: "p1"}, + WithHTTPClient(caller)) + if c2.httpClient == caller { + t.Fatal("client reused the caller's *http.Client — must build a fresh one (no copy-after-use)") + } + if c2.httpClient.CheckRedirect == nil { + t.Fatal("injected client's CheckRedirect was not overridden") + } + if err := c2.httpClient.CheckRedirect(nil, nil); err != http.ErrUseLastResponse { + t.Errorf("injected client's CheckRedirect = %v, want http.ErrUseLastResponse (unconditional override)", err) + } + if c2.httpClient.Transport != callerTransport { + t.Error("fresh client did not preserve the caller's Transport") + } + if c2.httpClient.Timeout != 17*time.Second { + t.Errorf("fresh client Timeout = %v, want the caller's 17s", c2.httpClient.Timeout) + } + // The caller's client is untouched. + if err := caller.CheckRedirect(nil, nil); err != sentinel { + t.Errorf("caller's CheckRedirect was mutated: got %v, want the untouched sentinel", err) + } +} + +// TestCreateOutcomeAmbiguous_3xxIsAmbiguous verifies a mutating 3xx (now surfaced +// as an APIError because redirect following is disabled) is classified AMBIGUOUS +// alongside 5xx — Meta may have committed the create before returning the redirect, +// so a caller must not treat it as a definite failure and blind-retry (duplicate). +// A definite 4xx stays non-ambiguous. +func TestCreateOutcomeAmbiguous_3xxIsAmbiguous(t *testing.T) { + cases := []struct { + status int + want bool + }{ + {http.StatusFound, true}, // 302 — redirect, not followed + {http.StatusTemporaryRedirect, true}, // 307 + {http.StatusInternalServerError, true}, // 500 + {http.StatusBadGateway, true}, // 502 + {http.StatusBadRequest, false}, // 400 — definite rejection + {http.StatusNotFound, false}, // 404 + {http.StatusTooManyRequests, false}, // 429 handled by retry, not here + } + for _, tc := range cases { + err := &APIError{StatusCode: tc.status, Method: http.MethodPost, Path: "/campaigns"} + if got := createOutcomeAmbiguous(err); got != tc.want { + t.Errorf("createOutcomeAmbiguous(APIError{%d}) = %v, want %v", tc.status, got, tc.want) + } + } + + // The 3xx-ambiguity is gated on a MUTATING method: a GET redirect is not a + // create, so it must NOT be UNCONFIRMED. A 5xx stays ambiguous regardless of + // method (the server may have committed before erroring). Matches the reddit + // client's contract. + methodCases := []struct { + method string + status int + want bool + }{ + {http.MethodGet, http.StatusFound, false}, // GET 302 — not a create + {http.MethodGet, http.StatusInternalServerError, true}, // GET 500 — still ambiguous + {http.MethodPost, http.StatusFound, true}, // POST 302 — mutating redirect + {http.MethodDelete, http.StatusTemporaryRedirect, true}, // DELETE 307 — mutating + } + for _, tc := range methodCases { + err := &APIError{StatusCode: tc.status, Method: tc.method, Path: "/campaigns"} + if got := createOutcomeAmbiguous(err); got != tc.want { + t.Errorf("createOutcomeAmbiguous(APIError{%s %d}) = %v, want %v", tc.method, tc.status, got, tc.want) + } + } +} diff --git a/internal/platform/twitter/client.go b/internal/platform/twitter/client.go index 9b0f0e57..40b05f1f 100644 --- a/internal/platform/twitter/client.go +++ b/internal/platform/twitter/client.go @@ -122,6 +122,19 @@ type Client struct { // Option customizes a Client at construction time. type Option func(*Client) +// noFollow is the CheckRedirect policy for every client this package uses: it +// returns http.ErrUseLastResponse so the client does NOT follow redirects and +// hands the 3xx response back to the request layer, where a non-2xx status is +// surfaced as an error. The X Ads API returns JSON directly and never legitimately +// 3xx-redirects these calls; not following keeps outcome classification sound — a +// redirect can't carry an already-sent mutating POST to a different target (and, +// with OAuth 1.0a, a followed redirect would resend a request signed for the +// original URL to a different one). Shared by the built-in client and the caller- +// supplied-client enforcement in NewClient. Mirrors the reddit/googleads clients. +func noFollow(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse +} + // WithBaseURL overrides the API base URL (default DefaultBaseURL). Trailing // slashes are trimmed so accountURL never produces a double-slash path (e.g. // "https://ads-api.x.com/" + "/12/..." -> "//12/..."), which would be signed @@ -165,7 +178,7 @@ func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client account: account, baseURL: DefaultBaseURL, apiVersion: DefaultAPIVersion, - httpClient: &http.Client{Timeout: requestTimeout}, + httpClient: &http.Client{Timeout: requestTimeout, CheckRedirect: noFollow}, nonceFn: defaultNonce, timeFn: time.Now, writeDelay: writeDelay, @@ -173,6 +186,17 @@ func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client for _, o := range opts { o(c) } + // Enforce the no-follow redirect policy UNCONDITIONALLY on whatever client ended + // up on c.httpClient — INCLUDING one supplied via WithHTTPClient. Following a + // redirect would carry an already-sent mutating POST to a different target (and + // resend an OAuth-1.0a request signed for the original URL), so no-follow is a + // correctness requirement, not a default. Applied to a SHALLOW COPY so the + // caller's *http.Client is never mutated. Mirrors the reddit client. + if c.httpClient != nil { + hc := *c.httpClient + hc.CheckRedirect = noFollow + c.httpClient = &hc + } return c } diff --git a/internal/platform/twitter/client_test.go b/internal/platform/twitter/client_test.go index dfdea21a..77ff95a5 100644 --- a/internal/platform/twitter/client_test.go +++ b/internal/platform/twitter/client_test.go @@ -9,6 +9,7 @@ import ( "crypto/sha1" //nolint:gosec // OAuth 1.0a mandates HMAC-SHA1; test mirrors production signing. "encoding/base64" "encoding/json" + "errors" "io" "math" "net/http" @@ -2736,3 +2737,43 @@ func stepsContain(steps []string, substr string) bool { } return false } + +// TestNoFollowRedirectPolicy verifies the client force-disables redirect +// following: the default client gets CheckRedirect=noFollow, and a WithHTTPClient- +// supplied client is also overridden — on a copy, so the caller's client is not +// mutated. With OAuth 1.0a a followed redirect would also resend a request signed +// for the original URL to a different one. +func TestNoFollowRedirectPolicy(t *testing.T) { + creds := Credentials{ConsumerKey: "ck", ConsumerSecret: "cs", AccessToken: "at", AccessTokenSecret: "ats"} + acct := AccountConfig{AccountID: "acc1", FundingInstrumentID: "fi1"} + + c := NewClient(creds, acct) + if c.httpClient.CheckRedirect == nil { + t.Fatal("default client has no CheckRedirect — redirects would be followed") + } + if err := c.httpClient.CheckRedirect(nil, nil); err != http.ErrUseLastResponse { + t.Errorf("CheckRedirect = %v, want http.ErrUseLastResponse (no-follow)", err) + } + + // Inject a client that ALREADY carries a caller-supplied redirect policy (a + // sentinel). This distinguishes an unconditional override from a "fill only nil + // callbacks" implementation: the latter would preserve the sentinel and silently + // re-enable redirect following. We assert (a) the client the code actually uses + // force-returns http.ErrUseLastResponse despite the sentinel, and (b) the + // caller's original client is untouched (shallow copy, not mutation). + sentinel := errors.New("caller-sentinel-redirect-policy") + caller := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return sentinel }} + c2 := NewClient(creds, acct, WithHTTPClient(caller)) + if c2.httpClient.CheckRedirect == nil { + t.Fatal("injected client's CheckRedirect was not overridden") + } + if err := c2.httpClient.CheckRedirect(nil, nil); err != http.ErrUseLastResponse { + t.Errorf("injected client's CheckRedirect = %v, want http.ErrUseLastResponse (unconditional override)", err) + } + if caller.CheckRedirect == nil { + t.Fatal("caller's *http.Client was mutated — override must use a shallow copy") + } + if err := caller.CheckRedirect(nil, nil); err != sentinel { + t.Errorf("caller's CheckRedirect was mutated: got %v, want the untouched sentinel", err) + } +}