fix(providers/azure): harden Synapse nil HTTP client fallback

[cristim]

Problem

Code-review finding SEC-03 (codebase-review-2026-06-10): the Azure Synapse client's NewClientWithHTTP constructor was the only one of the Azure NewClientWithHTTP constructors that fell back to http.DefaultClient on nil injection. http.DefaultClient has no IMDS-blocking transport, so a future non-test NewClientWithHTTP(..., nil) caller would reopen the SSRF surface the hardened httpclient.New() client closes.

Fix

Replace the nil fallback with httpclient.New() (SSRF/IMDS-blocking), matching the production NewClient constructor and the established pattern already used by the savingsplans and managedredis clients, and update the doc comment accordingly.

Test evidence

• Added TestNewClientWithHTTP_NilFallbackIsHardened, mirroring the savingsplans regression test: asserts the nil fallback is non-nil, is not http.DefaultClient, and that a request to http://169.254.169.254/metadata/instance is rejected with a "blocked" error.
• Confirmed the test FAILS on pre-fix code (fallback equals http.DefaultClient) and passes post-fix.
• go build ./... succeeds in both the root module and the providers/azure module; go test ./services/synapse/ -count=1 passes (44 tests).

Closes #1143

Summary by CodeRabbit

• Bug Fixes

  • Enhanced HTTP client security in Azure Synapse services with improved request protection.
  • Updated documentation in error handling logic.

• Tests

  • Added regression test to validate security hardening of HTTP client behavior.

[coderabbitai]

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8e07291a-d982-432b-b7a2-39f3c1d2c0b4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

NewClientWithHTTP in the Synapse client now substitutes httpclient.New() (the SSRF-hardened transport) instead of http.DefaultClient when the httpClient argument is nil. A comment in GetOfferingDetails is updated. A new regression test confirms the nil fallback rejects requests to the Azure IMDS endpoint with a "blocked" error.

Changes

SSRF Hardening for Nil HTTP Client Fallback

Layer / File(s)	Summary
Harden nil fallback and regression test providers/azure/services/synapse/client.go, providers/azure/services/synapse/client_test.go	NewClientWithHTTP replaces http.DefaultClient with httpclient.New() as the nil-argument fallback, adds an explanatory comment. A comment in GetOfferingDetails is also normalized. TestNewClientWithHTTP_NilFallbackIsHardened verifies the fallback client is non-nil, differs from http.DefaultClient, and returns a "blocked" error for IMDS requests.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

A rabbit once found a wide-open door,
Where DefaultClient let IMDS through before.
🐇 With httpclient.New() we slammed it shut tight,
And wrote a small test to confirm blocked delight.
No sneaky SSRF shall pass on our watch tonight! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely summarizes the main change: hardening the Synapse client's nil HTTP client fallback from http.DefaultClient to the SSRF-hardened httpclient.New().
Linked Issues check	✅ Passed	The PR fully addresses the coding requirements from issue #1143: replaces http.DefaultClient with httpclient.New() in NewClientWithHTTP and includes a regression test verifying IMDS endpoint rejection.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the security issue: the fallback replacement, regression test, comment correction, and response body closure are all directly related to the SSRF hardening objective.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/sec-03-fix

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Adversarial review (PR #1228)

Reviewed against the SEC-03 finding (#1143) and the five risk surfaces from the brief. Fix is correct, narrowly scoped, well tested. Nothing to push here; one defensive-consistency follow-up filed.

Verified

• The SEC-03 vulnerability is closed: providers/azure/services/synapse/client.go:69 now does httpClient = httpclient.New() instead of http.DefaultClient. httpclient.New() returns an *http.Client with a blockIMDSDialer transport that rejects 169.254.169.254 and fd00:ec2::254 (providers/azure/internal/httpclient/httpclient.go:26-35), closing the SSRF surface a future NewClientWithHTTP(..., nil) caller would otherwise reopen. The updated doc comment matches the new behaviour. ✓
• Sibling-provider sweep: grep -rn "http.DefaultClient" providers/azure/ now hits only the regression test (assertions + comment). grep -rn "&http.Client{" providers/azure/ hits only the legitimate factory in httpclient.go:49 -- the cross-cutting rule from feedback_azure_use_httpclient_new.md is respected. ✓
• NewClientWithHTTP nil-fallback shape across the 8 Azure providers:
  • 3 of 8 hardened (this PR's pattern): savingsplans, synapse, managedredis -- all guard with if httpClient == nil { httpClient = httpclient.New() }.
  • 5 of 8 still blindly assign: database, cache, search, cosmosdb, compute. These would nil-pointer-panic on .Do(req) rather than silently SSRF -- so not a SEC-03 regression -- but the inconsistency is now load-bearing in the opposite direction and could mislead future copy-paste into re-introducing the http.DefaultClient fallback. Filed #1304 (type/chore, priority/p3, severity/low, urgency/eventually, impact/internal, effort/xs, triaged). Out of scope here; SEC-03's body explicitly scoped the fix to Synapse.
• No production caller passes nil to NewClientWithHTTP: grep -rn "synapse.NewClientWithHTTP" --include='*.go' excluding tests returns nothing; the only non-test call site for the Synapse client is providers/azure/services.go:55 which uses synapse.NewClient(...) (which always builds via httpclient.New()). The SEC-03 finding's "Latent: no production caller passes nil today" assessment still holds. ✓
• feedback_preserve_caller_httpclient.md not violated: the new branch only fires when the caller passed nil -- there is no caller-supplied *http.Client whose Transport could be silently lost. When a caller passes a non-nil HTTPClient interface they control the implementation end-to-end (this is HTTPClient interface, not *http.Client). ✓
• feedback_no_silent_fallbacks.md borderline-OK: the fallback DOES silently substitute a hardened client for nil rather than returning an error. Acceptable here because (a) NewClientWithHTTP is a "for testing" constructor with no production nil callers, (b) the doc comment explicitly documents the substitution and the security rationale, and (c) erroring would force a non-trivial migration in the production NewClient wrapper. The non-money-path, defensive-default character lines up with how savingsplans and managedredis already handle it. ✓
• Regression test correctness (client_test.go:862-875): matches the savingsplans / managedredis pattern and adds an explicit require.NotEqual(t, http.DefaultClient, c.httpClient, "nil fallback must never be http.DefaultClient") — stronger than the sibling tests because it directly asserts the pre-fix sentinel, so future drift back to http.DefaultClient will fail the test loudly. Confirmed it FAILS on pre-fix code (Should not be: &http.Client{Transport:http.RoundTripper(nil),...}) and PASSES post-fix. Full synapse suite green (44/44, including -race). resp.Body.Close() is nil-guarded -- correct since the body should always be nil on dialer reject, but defensive close is fine. ✓
• Lint commits (375f24b1, 944d3c20) address the right thing: unrecognised → unrecognized is the existing repo spelling convention; bodyclose is addressed with a deferred nil-guarded defer resp.Body.Close() per feedback_nolint-style policy rather than a //nolint:bodyclose suppression. ✓

Fixes pushed

None — the diff stands as-is. Fix is correct, regression test is solid, and CR has zero inline findings (gh api repos/LeanerCloud/CUDly/pulls/1228/comments returns []).

Follow-up filed

• chore(providers/azure): align 5 sibling NewClientWithHTTP constructors with SEC-03 nil-fallback pattern #1304 — chore(providers/azure): align 5 sibling NewClientWithHTTP constructors with SEC-03 nil-fallback pattern. Adds the same if httpClient == nil { httpClient = httpclient.New() } guard + regression test to the 5 sibling Azure constructors (database, cache, search, cosmosdb, compute), so the cross-cutting feedback_azure_use_httpclient_new.md pattern lines up across all 8 services. Triage: type/chore, priority/p3, severity/low, urgency/eventually, impact/internal, effort/xs, triaged.

UNSTABLE state

Three failing checks (Lint Code, Integration Tests, Security Scanning, plus the CI Success aggregator) are pre-existing on main — the latest CI - Build & Test run on main (27833571269 on 451a70f7) fails the identical four jobs. None caused by this 3-file synapse diff. Not blocking.

Verdict

LGTM. CR re-ping below.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.