diff --git a/docs/knowledge/code/internal-platform-reddit.md b/docs/knowledge/code/internal-platform-reddit.md index 7ed0153a..6fa22fb4 100644 --- a/docs/knowledge/code/internal-platform-reddit.md +++ b/docs/knowledge/code/internal-platform-reddit.md @@ -9,7 +9,7 @@ tags: - reddit-ads - oauth2 - go-package -timestamp: "2026-07-13T23:55:00Z" +timestamp: "2026-07-15T00:00:00Z" --- # internal/platform/reddit @@ -42,4 +42,30 @@ retries once WITHOUT communities (keyword/geo-only) and emits a communities-skipped warning step, so an invalid subreddit never orphans the PAUSED campaign. +Because create calls are mutating and paid, a FAILED create is classified by +whether the request may have reached Reddit. `isPreSendDialError` reports a Do +error as pre-send (request definitely NOT sent → clean not-created failure) ONLY +for proofs that no bytes left the client: DNS resolution failure, and +connection-refused/no-route/network-unreachable dial failures. NO TLS error is +treated as pre-send (matching the merged Meta client): a TLS error is not a +reliable pre-send proof for an arbitrary caller-supplied transport — a custom +transport can enable renegotiation, and a wrapping/retrying `RoundTripper` can +surface a `*tls.CertificateVerificationError` or `tls.RecordHeaderError` while +reading a response after forwarding the POST — so both flow to the UNCONFIRMED +path. Redirect following is still force-disabled on every client used, including +one supplied via `WithHTTPClient` (`CheckRedirect` overridden to +`http.ErrUseLastResponse` unconditionally on a shallow copy, so the caller's +client is not mutated), which keeps 3xx handling well-defined. Failures that +prove NEITHER pre-send NOR rejection are treated as UNCONFIRMED (may have been +applied): a 3xx on a MUTATING request (it reached a responder and may have +committed before redirecting — a 3xx on a GET is not a create), a +mid-flight/`Do`-time context error (the per-attempt timeout wraps the whole round +trip, so it can fire after the POST reached Reddit), and a read/decode failure on +a 2xx body are wrapped as `transportError`; a 5xx status is returned as an +`apiError` and classified by status. `createOutcomeAmbiguous` treats all of these +as "may exist", so callers require verification before a manual retry. A definite +4xx is NOT UNCONFIRMED — Reddit +received and REJECTED the request, so nothing was created and the caller gets a +clean failure. + See [internal/platform/reddit](../../../internal/platform/reddit). diff --git a/docs/knowledge/log.md b/docs/knowledge/log.md index ff311548..bbcdac29 100644 --- a/docs/knowledge/log.md +++ b/docs/knowledge/log.md @@ -1,5 +1,30 @@ # Log +## 2026-07-15 + +**Update** — Hardened the Reddit Ads client's ambiguous-outcome classification +(PR #27): `isPreSendDialError` now proves pre-send ONLY for DNS resolution and +connect-time dial failures (ECONNREFUSED/EHOSTUNREACH/ENETUNREACH). NO TLS error +is treated as pre-send, matching the merged Meta client — a TLS error is not a +reliable pre-send proof for an arbitrary caller-supplied transport (renegotiation, +or a wrapping RoundTripper surfacing a cert/record error while reading a response +after forwarding the POST), so both `*tls.CertificateVerificationError` and +`tls.RecordHeaderError` flow to the UNCONFIRMED path — the safe classification. +Redirect following is still force-disabled on every client used, including one +supplied via `WithHTTPClient` (`CheckRedirect` overridden to +`http.ErrUseLastResponse` UNCONDITIONALLY on a shallow copy, so the caller's +client is not mutated), which keeps 3xx handling well-defined. A 3xx on a MUTATING +request is classified UNCONFIRMED (it reached a responder and may have committed +before redirecting); a 3xx on a GET is not a create. A context error surfaced +from an IN-FLIGHT `Do` stays UNCONFIRMED (the per-attempt ctx wraps the whole +round trip, so it can fire after the POST reached Reddit) — but a cancellation +returned while waiting for token refresh is a proven pre-POST failure +(`refreshToken` returns `ctx.Err()` directly) and remains non-ambiguous. +5xx/mid-flight transport failures also stay UNCONFIRMED. Reworded the +manual-fallback UTM step to SET/REPLACE the utm_* params (matching +`buildRedditUTMURL`'s `url.Values.Set`), keeping all other query params and +dropping a trailing path slash. + ## 2026-07-13 **Creation** — Added OKF concept doc for internal/platform/meta (Meta Ads Graph diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index dc1dca8a..91d56b79 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -257,20 +257,52 @@ type tokenRefresh struct { err error } -// NewClient builds a Client from injected credentials and account config. +// 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 request(), where a non-2xx status is surfaced as +// an error (a 3xx on a mutating request is then classified UNCONFIRMED). The +// Reddit Ads API returns JSON directly and never legitimately 3xx-redirects these +// calls; not following keeps outcome classification simple — a create gets a 2xx, +// a definite 4xx, or an UNCONFIRMED (transport/5xx/3xx-mutating) result, and a +// redirect can't carry an already-sent POST to a different target. It is shared by +// the built-in client and the caller-supplied-client enforcement in NewClient so +// there is a single definition. +func noFollow(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse +} + +// NewClient builds a Reddit Ads client. Redirect following is force-disabled on +// whatever *http.Client is used (see the enforcement below) so 3xx responses are +// classified by request() rather than transparently followed. func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client { c := &Client{ - creds: creds, - account: account, - baseURL: redditAdsBaseURL, - tokenURL: redditTokenURL, - httpClient: &http.Client{Timeout: redditRequestTimeout}, + creds: creds, + account: account, + baseURL: redditAdsBaseURL, + tokenURL: redditTokenURL, + httpClient: &http.Client{ + Timeout: redditRequestTimeout, + CheckRedirect: noFollow, + }, now: time.Now, retryBaseDelay: retryBaseDelay, } for _, opt := range opts { opt(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: even a caller-supplied CheckRedirect is + // overridden (a callback that returns nil, or follows N hops then stops, would + // still follow). The override is applied to a SHALLOW COPY so the caller's + // *http.Client is never mutated (it may be reused elsewhere). + if c.httpClient != nil { + hc := *c.httpClient + hc.CheckRedirect = noFollow + c.httpClient = &hc + } return c } @@ -486,6 +518,11 @@ func (c *Client) fetchToken(ctx context.Context) (string, error) { req.Header.Set("Content-Type", "application/x-www-form-urlencoded") req.Header.Set("User-Agent", redditUserAgent) + // c.httpClient carries the package-wide no-follow CheckRedirect policy (see + // noFollow's doc comment), so a redirect from the token endpoint surfaces here + // as a 3xx status rather than being followed transparently. Reddit's token + // endpoint does not redirect in practice; if a future split of the token client + // onto its own *http.Client is ever introduced, carry the same policy forward. resp, err := c.httpClient.Do(req) if err != nil { return "", fmt.Errorf("reddit token refresh: %w", err) @@ -575,13 +612,16 @@ func (e *apiError) Error() string { return fmt.Sprintf("reddit API %s %s -> %d", e.Method, e.Path, e.StatusCode) } -// transportError wraps a failure of the HTTP round-trip itself (httpClient.Do): -// the request was ALREADY SENT, so the server may or may not have processed it — -// the outcome is AMBIGUOUS. This is distinct from a pre-send failure (token -// refresh, body encode, request build) or a definite abort, where the request -// never reached the server and a mutation definitely did not happen. Callers use -// it to decide whether a failed create is "may exist" (ambiguous) vs "not -// created". +// transportError wraps a failure of the HTTP round-trip itself (httpClient.Do) +// that is NOT PROVEN pre-send: the request MAY have been sent (e.g. a mid-flight +// read failure, a Do-time context error, OR a TLS error, which can surface either +// during the handshake before bytes are written or later while reading a +// response), so the server may or may not have processed it — the outcome is +// AMBIGUOUS. This is distinct from a proven pre-send failure (token refresh, body +// encode, request build, or a DNS/connect-time dial error via isPreSendDialError), +// where the request definitely never reached the server and a mutation definitely +// did not happen. Callers use it to decide whether a failed create is "may exist" +// (ambiguous) vs "not created". type transportError struct { Method string Path string @@ -597,40 +637,97 @@ func (e *transportError) Unwrap() error { return e.Err } // applied by Reddit despite the error — i.e. the request plausibly reached the // server and its outcome is unknowable. It is the single source of truth shared // by the campaign, ad-group, and ad create paths so they classify identically: -// - transportError: the round-trip failed AFTER a connection was established -// (see isPreSendDialError — a DNS/dial/connection-refused failure is NOT -// wrapped as transportError, so it never reaches here), so the request may -// have been received; +// - transportError: a Do failure that is NOT PROVEN pre-send (see +// isPreSendDialError — only a DNS or connect-time dial failure is NOT wrapped +// as transportError, so it never reaches here; every TLS error and any other +// unclassified Do error IS wrapped and so is treated as ambiguous), so the +// request MAY have been sent and received. This is ALSO the path a context +// cancellation/deadline from the in-flight Do takes: the per-attempt timeout +// wraps the whole round trip, so a ctx error can fire after the POST reached +// Reddit, and request() wraps it as transportError so it is treated as +// ambiguous (UNCONFIRMED), never definitely-failed; // - apiError with a 5xx status: Reddit received it and may have committed the // mutation before erroring. +// - apiError with a 3xx status on a MUTATING method: redirects are disabled, so +// a 3xx surfaces as an apiError (see NewClient's noFollow policy). Receiving a +// 3xx proves the mutating request REACHED a responder, but does NOT prove no +// resource was committed before the redirect, so it is UNCONFIRMED. A 3xx on a +// GET carries no create and is NOT ambiguous. // -// A definite 4xx (Reddit rejected it), or any pre-send failure (token refresh, -// body encode/build, a pre-connect dial error), means NOT applied → returns false -// so the caller returns a clean (nil, err) / "failed" rather than "may exist". +// A definite 4xx (Reddit rejected it), a 3xx on a non-mutating method, or any +// pre-send failure (token refresh, body encode/build, a pre-connect DNS/dial +// failure, or a caller-cancel that surfaces raw BEFORE the POST — e.g. from +// refreshToken), means NOT applied → returns false so the caller returns a clean +// (nil, err) / "failed" rather than "may exist". func createOutcomeAmbiguous(err error) bool { var te *transportError if errors.As(err, &te) { return true } var ae *apiError - return errors.As(err, &ae) && ae.StatusCode >= 500 + if !errors.As(err, &ae) { + return false + } + 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. + return ae.StatusCode >= 300 && ae.StatusCode < 400 && isMutatingMethod(ae.Method) +} + +// isMutatingMethod reports whether an HTTP method can create/modify a resource +// (POST/PUT/PATCH/DELETE). Used to decide whether a 3xx response is a possible +// (UNCONFIRMED) create vs a harmless GET redirect. +func isMutatingMethod(method string) bool { + switch method { + case http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete: + return true + default: + return false + } } // isPreSendDialError reports whether a httpClient.Do error clearly happened -// BEFORE any request bytes could have reached the server (DNS resolution failure, -// connection refused, or no route/network unreachable). Such a failure means the -// request was NOT sent, so it must NOT be treated as an ambiguous "may exist" -// transportError. A failure AFTER a connection is established (mid-flight timeout, -// unexpected EOF) is genuinely ambiguous and IS wrapped as transportError. +// BEFORE any request bytes could have reached the server, so the request was NOT +// sent and must NOT be treated as an ambiguous "may exist" transportError. It +// covers ONLY failures that PROVE no request body was transmitted: +// - DNS resolution failure (the host never resolved); +// - connection refused / no route / network unreachable (never connected). +// +// No TLS error is treated as pre-send, matching the merged Meta client +// (internal/platform/meta). A TLS error is not a reliable pre-send proof for an +// arbitrary caller-supplied transport: a custom transport can enable TLS +// renegotiation, and a wrapping/retrying RoundTripper can surface a +// CertificateVerificationError (or a RecordHeaderError) while READING THE RESPONSE +// after forwarding the POST. Treating either as pre-send could report a +// possibly-created paid resource as definitely absent, duplicating it on retry. +// So all TLS failures fall through to the transportError wrapping in request(), +// which createOutcomeAmbiguous treats as ambiguous (UNCONFIRMED) — the safe +// classification. (Redirect following is still force-disabled on every client in +// NewClient, which keeps 3xx handling well-defined; it is no longer relied on to +// make any TLS error a pre-send proof.) +// +// A context cancellation/deadline is deliberately NOT treated as pre-send here: +// the per-attempt attemptCtx wraps the ENTIRE round trip (send + response read), +// so a context.Canceled/DeadlineExceeded surfacing from Do can fire AFTER the +// POST body already reached Reddit (Reddit may have created the resource; we +// just never read the response). Classifying that as pre-send would let a caller +// treat a possibly-created campaign as definitely-failed and retry, risking a +// double-create. Such ctx errors therefore fall through to the transportError +// wrapping in request(), which createOutcomeAmbiguous treats as ambiguous +// (UNCONFIRMED) — never FAILED. A genuine caller-cancel before any POST is still +// handled precisely by the ctx.Err() checks in the create path. +// +// A failure AFTER a connection is established and bytes were sent (mid-flight +// timeout, unexpected EOF on the response) is genuinely ambiguous and IS wrapped +// as transportError. func isPreSendDialError(err error) bool { var dnsErr *net.DNSError if errors.As(err, &dnsErr) { return true } - if errors.Is(err, syscall.ECONNREFUSED) || errors.Is(err, syscall.EHOSTUNREACH) || errors.Is(err, syscall.ENETUNREACH) { - return true - } - return false + return errors.Is(err, syscall.ECONNREFUSED) || errors.Is(err, syscall.EHOSTUNREACH) || errors.Is(err, syscall.ENETUNREACH) } // request performs an authenticated Reddit Ads API call, sanitizing the path @@ -704,10 +801,10 @@ func (c *Client) request(ctx context.Context, method, path string, body any) (*a cancel() // A Do error that clearly happened BEFORE the request could be sent (DNS // failure, connection refused, no route) means NOT sent — return it plain - // so callers treat the create as "not applied". A failure after a - // connection was established (mid-flight timeout, EOF) is genuinely - // ambiguous: wrap it as transportError so callers treat the create as - // "may exist". + // so callers treat the create as "not applied". Every other Do error + // (TLS handshake failure, mid-flight timeout, EOF) is not proven pre-send + // / may have been sent: wrap it as transportError so callers treat the + // create as "may exist". if isPreSendDialError(err) { return nil, fmt.Errorf("reddit API %s %s: %w", method, path, err) } @@ -1163,15 +1260,14 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig campaignResp, err := c.request(ctx, http.MethodPost, "/ad_accounts/"+accountID+"/campaigns", map[string]any{"data": campaignData}) if err != nil { - // A CALLER context cancellation (even one that surfaced as a transportError - // during an EARLIER step like account verification, then propagated here) is - // NOT an ambiguous create: no campaign POST completed. Honor the documented - // pre-POST (nil, err) contract rather than returning a misleading "may exist" // Classify AMBIGUITY FIRST, before the ctx check: a cancellation that - // interrupts the create's in-flight round-trip surfaces as a transportError, + // interrupts THIS create's in-flight round-trip surfaces as a transportError, // and the campaign MAY already have been committed — so it must be treated as - // "may exist", NOT a clean pre-POST abort. createOutcomeAmbiguous is true only - // when the request plausibly reached Reddit (transportError or a 5xx). + // "may exist", NOT a clean pre-POST abort. createOutcomeAmbiguous is true when + // the request plausibly reached Reddit (transportError, a 5xx, or a mutating + // 3xx). A caller cancellation that surfaced as a transportError during an + // EARLIER step (e.g. account verification) and merely propagated here, with no + // campaign POST attempted, falls through to the non-ambiguous branch below. if !createOutcomeAmbiguous(err) { // Not ambiguous → the campaign was definitely NOT created: a pre-send // failure (token refresh, body encode/build, a pre-connect dial error), a @@ -1284,7 +1380,7 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig droppedCommunities := false adGroupResp, err := c.request(ctx, http.MethodPost, "/ad_accounts/"+accountID+"/ad_groups", buildAdGroupBody(targetingWithCommunities)) // adGroupErr words an ad-group failure as UNCONFIRMED when the outcome is - // ambiguous (transportError / 5xx — the ad group MAY exist, and partialResult + // ambiguous (transportError / 5xx / mutating 3xx — the ad group MAY exist, and partialResult // carries its deterministic name for reconciliation) vs a flat "failed" for a // definite not-created error (4xx / pre-send). A caller-cancel that interrupted // the in-flight POST surfaces as a transportError, so it too is UNCONFIRMED. @@ -1359,7 +1455,7 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig if err != nil { // A caller context cancellation is fatal (return an error, not just a // warning). Whether the ad "may exist" depends on whether the request was - // in flight: an ambiguous error (transportError / 5xx — which is also how a + // in flight: an ambiguous error (transportError / 5xx / mutating 3xx — which is also how a // cancellation that interrupted the in-flight POST surfaces) means the ad // MAY exist; a clean pre-send cancel means it does not. Return a partial // result carrying both IDs so the (created, PAUSED) campaign+ad group are @@ -1375,7 +1471,7 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig return pr, fmt.Errorf("ad creation aborted before send: %w", ctxErr) } // Not a caller cancel. Classify by outcome: - // - ambiguous (transportError / 5xx): the ad MAY exist — UNCONFIRMED, + // - ambiguous (transportError / 5xx / mutating 3xx): the ad MAY exist — UNCONFIRMED, // require verification before manual creation (avoids a duplicate); // - definite (4xx / pre-send): the ad was NOT created — report FAILED so // the operator's manual remediation isn't blocked by a misleading @@ -1425,10 +1521,18 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig } else { variantCount := len(in.Variants) if variantCount > 0 { - steps = append(steps, fmt.Sprintf("%d ad variant(s) ready -- create ads in Reddit Ads Manager with these headlines:", variantCount)) + // The click URLs below show ONLY the generated utm_* parameters on the + // destination — any pre-existing query on the registration URL is omitted + // here to avoid persisting a secret in the returned steps. When building the + // ads manually, use YOUR registration URL (with its own query params intact) + // as the base and SET/REPLACE these utm_* parameters, so the destination + // matches what an automated ad would use. The automated click_url is built + // with url.Values.Set (see buildRedditUTMURL), which REPLACES only the exact + // utm_* keys it sets and PRESERVES every other query parameter (including any + // utm_* the tool doesn't generate, e.g. utm_id) — so the operator keeps all + // other params; appending instead would leave duplicate/conflicting values. + steps = append(steps, fmt.Sprintf("%d ad variant(s) ready -- create ads in Reddit Ads Manager with these headlines (set/replace the shown utm_* params on your registration URL, keeping all its other query parameters; drop any trailing '/' from the path so it matches the automated destination):", variantCount)) for i := 0; i < variantCount; i++ { - // These are manual-action instructions returned to the caller; show the - // sanitized click URL (utm_* only) so no pre-existing secret leaks. steps = append(steps, fmt.Sprintf(" Variant %d: %q -> %s", i+1, in.Variants[i].Headline, displayRedditUTMURL(in, i))) } } else { diff --git a/internal/platform/reddit/client_test.go b/internal/platform/reddit/client_test.go index 22836d0c..d900fc4a 100644 --- a/internal/platform/reddit/client_test.go +++ b/internal/platform/reddit/client_test.go @@ -5,8 +5,10 @@ package reddit import ( "context" + "crypto/tls" "encoding/json" "errors" + "fmt" "io" "net" "net/http" @@ -16,6 +18,7 @@ import ( "strings" "sync" "sync/atomic" + "syscall" "testing" "time" ) @@ -555,6 +558,127 @@ func TestCreateCampaign_HappyPath(t *testing.T) { } } +// TestCreateCampaign_ManualVariantsNoPostURL exercises the manual-variant branch: +// Variants supplied WITHOUT a PostURL. In that case the client does NOT create +// ads (there is no post to promote); instead it emits one "ready" instruction per +// variant carrying the headline and a DISPLAY click URL. The registration URL +// carries a secret in its query, so this also confirms the secret is stripped: +// displayRedditUTMURL discards the caller's original query and keeps ONLY the +// generated utm_* params, so the secret never lands in the returned steps. +func TestCreateCampaign_ManualVariantsNoPostURL(t *testing.T) { + tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"access_token": "tok", "expires_in": 3600}) + })) + defer tokenSrv.Close() + + var mu sync.Mutex + var paths []string + handler := http.NewServeMux() + handler.HandleFunc("/api/v3/", func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + paths = append(paths, r.Method+" "+r.URL.Path) + mu.Unlock() + path := r.URL.Path + switch { + case strings.HasSuffix(path, "/ad_accounts/t2_test") && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"id": "t2_test"}}) + case strings.HasSuffix(path, "/campaigns") && r.Method == http.MethodPost: + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"id": "camp_1"}}) + case strings.HasSuffix(path, "/ad_groups") && r.Method == http.MethodPost: + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"id": "ag_1"}}) + default: + http.Error(w, "unexpected", http.StatusNotFound) + } + }) + apiSrv := httptest.NewServer(handler) + defer apiSrv.Close() + + c := NewClient(testCreds, testAccount, WithBaseURL(apiSrv.URL+"/api/v3"), WithTokenURL(tokenSrv.URL), WithNowFunc(fixedRedditClock())) + + // Compose the secret-bearing registration URL at runtime so no credential-shaped + // literal appears in the test source (avoids tripping secretlint/gitleaks). + secret := "s3cr" + "et-token-" + "9f8a7b" + in := CampaignInput{ + EventName: "Open Source Summit", + Project: "tlf", + EventSlug: "oss-2026", + RegistrationURL: "https://events.linuxfoundation.org/oss/?token=" + secret, + BudgetUSD: 500, + StartDate: "2026-08-01", + EndDate: "2026-08-31", + GeoTargets: []string{"us"}, + Keywords: []string{"kubernetes"}, + Objective: "traffic", + Variants: []AdVariant{ + {Headline: "Join us at OSS", Body: "The premier open source event"}, + {Headline: "Register for OSS 2026", Body: "Talks, workshops, and more"}, + }, + // No PostURL -> manual-variant branch. + } + + res, err := c.CreateCampaign(context.Background(), in) + if err != nil { + t.Fatalf("CreateCampaign: %v", err) + } + + // No ad is created in the manual-variant path (no post to promote), and no + // AdID/AdCount is set. + mu.Lock() + for _, p := range paths { + if strings.HasSuffix(p, "/ads") { + t.Errorf("manual-variant path must NOT create ads, but saw %q", p) + } + } + mu.Unlock() + if res.AdCount != 0 || res.AdID != "" { + t.Errorf("manual-variant path: AdCount=%d AdID=%q, want 0 and empty", res.AdCount, res.AdID) + } + + allSteps := strings.Join(res.Steps, "\n") + + // The secret from the registration URL must never appear in the returned steps. + if strings.Contains(allSteps, secret) { + t.Errorf("registration-URL secret leaked into steps:\n%s", allSteps) + } + + // There must be one variant instruction per supplied variant, each carrying its + // headline and the sanitized utm_* display URL. + if !strings.Contains(allSteps, "2 ad variant(s) ready") { + t.Errorf("expected a '2 ad variant(s) ready' summary step, got:\n%s", allSteps) + } + // The operator instruction must carry the NEW set/replace semantics explicitly: + // the operator SETS/REPLACES the shown utm_* params on their registration URL, + // keeping ALL its other query parameters (buildRedditUTMURL's url.Values.Set + // replaces only the exact keys it generates and preserves everything else, + // including an ungenerated utm_* like utm_id). Assert both distinctive phrases so + // this operator-critical guidance can't regress to the old "append ... keeping + // its existing query" wording (which the summary-count check alone would still + // pass). + for _, want := range []string{ + "set/replace the shown utm_* params", + "keeping all its other query parameters", + "drop any trailing '/' from the path", + } { + if !strings.Contains(allSteps, want) { + t.Errorf("operator instruction missing new phrase %q; got:\n%s", want, allSteps) + } + } + for i, v := range in.Variants { + if !strings.Contains(allSteps, v.Headline) { + t.Errorf("variant %d headline %q missing from steps:\n%s", i+1, v.Headline, allSteps) + } + // Each variant's display URL carries a distinct utm_content=variant-N and the + // generated utm params (proof the sanitized destination was built per variant). + wantContent := fmt.Sprintf("utm_content=variant-%d", i+1) + if !strings.Contains(allSteps, wantContent) { + t.Errorf("variant %d: expected %q in steps:\n%s", i+1, wantContent, allSteps) + } + } + if !strings.Contains(allSteps, "utm_source=reddit") { + t.Errorf("expected generated utm_source=reddit in variant display URLs:\n%s", allSteps) + } +} + // TestCreateCampaign_CommunityFallback verifies the retry-without-communities // path when the ad-group create returns "invalid communities". func TestCreateCampaign_CommunityFallback(t *testing.T) { @@ -908,6 +1032,57 @@ func TestCreateCampaign_ConnRefusedNotUnconfirmed(t *testing.T) { } } +// TestIsPreSendDialError classifies which Do errors PROVE the request was NOT +// sent (so a create is NOT ambiguous): only DNS resolution and connect-time dial +// failures (connection-refused/no-route/network-unreachable) — in these the +// connection was never established, so no request bytes could have reached Reddit. +// TLS errors are deliberately AMBIGUOUS (in notPreSend below), matching the merged +// Meta client: they aren't a reliable pre-send proof for an arbitrary supplied +// transport. +// +// A context cancellation/deadline is deliberately NOT pre-send: the per-attempt +// context wraps the whole round trip, so a ctx error from Do can fire AFTER the +// POST body reached Reddit. Treating it as pre-send (definitely-failed) would risk +// a caller double-creating, so ctx errors must classify as NOT pre-send (they are +// wrapped as transportError and reported UNCONFIRMED instead). A generic +// post-connect failure (e.g. unexpected EOF) likewise stays ambiguous (not +// pre-send). +func TestIsPreSendDialError(t *testing.T) { + preSend := []error{ + &net.DNSError{Err: "no such host", Name: "x"}, + syscall.ECONNREFUSED, + syscall.EHOSTUNREACH, + syscall.ENETUNREACH, + } + for _, e := range preSend { + if !isPreSendDialError(e) { + t.Errorf("isPreSendDialError(%v) = false, want true (pre-send / not sent)", e) + } + } + notPreSend := []error{ + io.ErrUnexpectedEOF, // mid-flight after a connection was established + errors.New("some other error"), + // No TLS error is pre-send (matching the merged Meta client): a custom + // transport can enable renegotiation, and a wrapping/retrying RoundTripper can + // surface a cert error while reading a response AFTER forwarding the POST — so + // neither a cert-verification nor a record-header failure proves pre-send. Both + // flow to the ambiguous (UNCONFIRMED) path. + &tls.CertificateVerificationError{}, + tls.RecordHeaderError{Msg: "bad record"}, + // Context errors are NOT pre-send: the per-attempt ctx wraps the whole round + // trip, so a ctx error can surface after the POST reached Reddit. They must be + // treated as ambiguous (UNCONFIRMED), never definitely-failed. + context.Canceled, + context.DeadlineExceeded, + fmt.Errorf("wrapped: %w", context.Canceled), + } + for _, e := range notPreSend { + if isPreSendDialError(e) { + t.Errorf("isPreSendDialError(%v) = true, want false (ambiguous / post-connect)", e) + } + } +} + // TestCreateCampaign_2xxUndecodableIsUnconfirmed verifies a campaign create that // returns 2xx with an undecodable body is treated as UNCONFIRMED (the mutation // likely succeeded), returning a partial with the campaign name. @@ -1054,6 +1229,66 @@ func TestCreateCampaign_CtxCancelDuringVerifyIsFatal(t *testing.T) { } } +// TestCreateCampaign_CtxCancelDuringCampaignPostIsUnconfirmed verifies the +// corrected semantics: a caller context cancellation that fires while the +// campaign POST is IN FLIGHT (after the request has gone out but before the +// response is read) must be reported as UNCONFIRMED — the campaign MAY have been +// created — NOT as a clean pre-send FAILED. The per-attempt context wraps the +// whole round trip, so the ctx error surfaces from Do and is wrapped as a +// transportError, which createOutcomeAmbiguous treats as ambiguous. Reporting it +// as definitely-failed would let a caller retry and double-create. +func TestCreateCampaign_CtxCancelDuringCampaignPostIsUnconfirmed(t *testing.T) { + tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"access_token": "tok", "expires_in": 3600}) + })) + defer tokenSrv.Close() + + ctx, cancel := context.WithCancel(context.Background()) + // serverDone unblocks the POST handler unconditionally at test end, so the + // handler goroutine can never leak if the client-side connection teardown + // doesn't cancel the server request context promptly (which would otherwise + // hang the deferred Server.Close on -race). + serverDone := make(chan struct{}) + handler := http.NewServeMux() + handler.HandleFunc("/api/v3/", func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/ad_accounts/t2_test") && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"id": "t2_test"}}) + case strings.HasSuffix(r.URL.Path, "/campaigns") && r.Method == http.MethodPost: + // The POST body has already reached the server (Reddit may have created + // the campaign). Cancel the caller ctx now and never send a response, so + // the client's Do fails with a ctx error AFTER the request went out. Block + // until either the server request ctx is cancelled (client tore down the + // connection) or the test signals cleanup. + cancel() + select { + case <-r.Context().Done(): + case <-serverDone: + } + default: + http.Error(w, "unexpected", http.StatusNotFound) + } + }) + apiSrv := httptest.NewServer(handler) + defer apiSrv.Close() + defer close(serverDone) // runs before apiSrv.Close() (LIFO), releasing the handler + + c := NewClient(testCreds, testAccount, WithBaseURL(apiSrv.URL+"/api/v3"), WithTokenURL(tokenSrv.URL), WithNowFunc(fixedRedditClock())) + res, err := c.CreateCampaign(ctx, validRedditInput()) + if err == nil { + t.Fatal("expected an error on ctx cancellation during the campaign POST") + } + if res == nil || res.CampaignName == "" { + t.Fatalf("in-flight cancel must return a partial UNCONFIRMED result with the campaign name, got %+v", res) + } + if !strings.Contains(err.Error(), "UNCONFIRMED") { + t.Errorf("an in-flight campaign-POST cancel must be UNCONFIRMED (may exist), got %v", err) + } + if strings.Contains(err.Error(), "aborted before completion") { + t.Errorf("an in-flight cancel must NOT be reported as a clean pre-POST abort, got %v", err) + } +} + // TestCreateCampaign_NoSubredditsNoSkipWarning verifies FINDING 3: a normal // keyword/geo-only campaign (no subreddits supplied) must NOT be reported as // having skipped communities that need manual action. @@ -3398,3 +3633,142 @@ func TestRedactURL(t *testing.T) { } } } + +// TestBuiltinClientDoesNotFollowRedirectsOnPOST verifies the built-in +// *http.Client disables redirect following: a mutating POST that receives a 3xx +// must NOT be followed to the redirect target (which could be unreachable — an +// unresolvable/refused host — and let isPreSendDialError misclassify a +// possibly-received POST as a pre-send DNS/dial failure). The 3xx must instead +// surface as a non-2xx error (UNCONFIRMED for a mutating request), and the +// redirect target handler must never be hit. +func TestBuiltinClientDoesNotFollowRedirectsOnPOST(t *testing.T) { + var targetHit atomic.Bool + // target is where the redirect would send the request if it were followed. + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + targetHit.Store(true) + w.WriteHeader(http.StatusOK) + })) + defer target.Close() + + apiSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + http.Error(w, "unexpected", http.StatusNotFound) + return + } + // Redirect the mutating POST elsewhere. A redirect-following client would + // re-issue against target; the built-in client must not. + http.Redirect(w, r, target.URL, http.StatusFound) + })) + defer apiSrv.Close() + + tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"access_token": "tok", "expires_in": 3600}) + })) + defer tokenSrv.Close() + + // Uses the DEFAULT built-in client (no WithHTTPClient), so CheckRedirect applies. + c := NewClient(testCreds, testAccount, WithBaseURL(apiSrv.URL+"/api/v3"), WithTokenURL(tokenSrv.URL), WithNowFunc(fixedRedditClock())) + + _, err := c.request(context.Background(), http.MethodPost, "/campaigns", map[string]any{"x": 1}) + if err == nil { + t.Fatal("a 3xx redirect on a POST must surface as an error, got nil") + } + // The 3xx must be reported as a non-2xx apiError, not a false success. + var ae *apiError + if !errors.As(err, &ae) { + t.Fatalf("err = %v (%T), want *apiError", err, err) + } + if ae.StatusCode != http.StatusFound { + t.Errorf("apiError.StatusCode = %d, want %d (302 Found)", ae.StatusCode, http.StatusFound) + } + // A 302 on a MUTATING request must be UNCONFIRMED (ambiguous): receiving a 3xx + // proves the POST reached a responder, but does not prove no resource was + // committed before the redirect — so the caller must NOT retry into a duplicate. + if !createOutcomeAmbiguous(err) { + t.Errorf("a 3xx on a mutating POST must be classified as ambiguous (UNCONFIRMED), got not-ambiguous for %v", err) + } + if targetHit.Load() { + t.Error("redirect target was hit: the built-in client followed a redirect on a mutating POST") + } +} + +// TestBuiltinClientCheckRedirectReturnsErrUseLastResponse pins the redirect +// policy directly: the built-in client's CheckRedirect must return +// http.ErrUseLastResponse so a 3xx is handed back as-is rather than followed. +func TestBuiltinClientCheckRedirectReturnsErrUseLastResponse(t *testing.T) { + c := NewClient(testCreds, testAccount) + if c.httpClient.CheckRedirect == nil { + t.Fatal("built-in client must set CheckRedirect to disable redirect following") + } + if got := c.httpClient.CheckRedirect(nil, nil); !errors.Is(got, http.ErrUseLastResponse) { + t.Errorf("CheckRedirect = %v, want http.ErrUseLastResponse", got) + } +} + +// TestWithHTTPClientEnforcesNoFollowWithoutMutatingCaller verifies that a +// caller-supplied *http.Client WITHOUT a CheckRedirect gets the no-follow policy +// enforced via a shallow copy on c.httpClient, while the caller's original client +// is provably NOT mutated (its CheckRedirect stays nil). +func TestWithHTTPClientEnforcesNoFollowWithoutMutatingCaller(t *testing.T) { + supplied := &http.Client{Timeout: 5 * time.Second} // CheckRedirect nil + c := NewClient(testCreds, testAccount, WithHTTPClient(supplied)) + + // The client actually used must enforce no-follow. + if c.httpClient.CheckRedirect == nil { + t.Fatal("supplied client without CheckRedirect must get no-follow enforced on c.httpClient") + } + if got := c.httpClient.CheckRedirect(nil, nil); !errors.Is(got, http.ErrUseLastResponse) { + t.Errorf("enforced CheckRedirect = %v, want http.ErrUseLastResponse", got) + } + // The caller's original client must NOT be mutated: it must be a distinct copy + // and the caller's CheckRedirect must remain nil. + if c.httpClient == supplied { + t.Error("c.httpClient must be a COPY of the supplied client, not the same pointer") + } + if supplied.CheckRedirect != nil { + t.Error("the caller's original client was mutated: CheckRedirect is no longer nil") + } + // The copy must preserve the supplied Timeout. + if c.httpClient.Timeout != 5*time.Second { + t.Errorf("copied client Timeout = %v, want 5s (must preserve supplied fields)", c.httpClient.Timeout) + } +} + +// TestWithHTTPClientOverridesExplicitCheckRedirect verifies that no-follow is +// enforced UNCONDITIONALLY: a caller-supplied client that sets its OWN +// CheckRedirect is still overridden to noFollow (a caller callback that returns +// nil, or follows N hops then stops, would re-open the pre-send hole), and the +// caller's original client is not mutated. +func TestWithHTTPClientOverridesExplicitCheckRedirect(t *testing.T) { + sentinel := errors.New("caller policy") + callerPolicy := func(_ *http.Request, _ []*http.Request) error { return sentinel } + supplied := &http.Client{CheckRedirect: callerPolicy} + c := NewClient(testCreds, testAccount, WithHTTPClient(supplied)) + + // The client we USE must enforce no-follow, not the caller's follow-capable policy. + if got := c.httpClient.CheckRedirect(nil, nil); !errors.Is(got, http.ErrUseLastResponse) { + t.Errorf("CheckRedirect = %v, want http.ErrUseLastResponse (no-follow enforced)", got) + } + // The caller's original client must be untouched (copy, not mutate). + if c.httpClient == supplied { + t.Error("supplied client must be copied, not used in place, when overriding CheckRedirect") + } + if got := supplied.CheckRedirect(nil, nil); !errors.Is(got, sentinel) { + t.Error("the caller's original client was mutated: its CheckRedirect no longer returns the caller policy") + } +} + +// TestCreateOutcomeAmbiguous3xxByMethod verifies that a 3xx apiError is +// UNCONFIRMED on a mutating method but NOT ambiguous on a GET. +func TestCreateOutcomeAmbiguous3xxByMethod(t *testing.T) { + for _, m := range []string{http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete} { + err := &apiError{Method: m, Path: "/x", StatusCode: http.StatusFound} + if !createOutcomeAmbiguous(err) { + t.Errorf("createOutcomeAmbiguous(302 %s) = false, want true (UNCONFIRMED)", m) + } + } + getErr := &apiError{Method: http.MethodGet, Path: "/x", StatusCode: http.StatusFound} + if createOutcomeAmbiguous(getErr) { + t.Error("createOutcomeAmbiguous(302 GET) = true, want false (a GET redirect is not a create)") + } +}