mirror of
https://github.com/netbirdio/netbird.git
synced 2026-08-27 18:11:29 +02:00
67b1373dfe
Five smaller points from the same review as the four already fixed. **The no-redirect policy had no test.** Every Fetch test injects an HTTPClient, which bypasses httpClient() and therefore the policy entirely, so nothing asserted that a 302 is refused — and the policy is a security control: a redirect moves the request to a host checkPublicHost never resolved. TestRedirectsAreNotFollowed drives the real constructor against an httptest server that redirects to the cloud metadata address, and asserts exactly one request leaves the client. **An injected client silently lost that policy.** Production is safe today only because NewManager passes a nil HTTPClient; any future non-test injection would have dropped the guarantee with no signal. An injected client that states no policy now inherits ours. Implemented by copying the client rather than assigning into it. Writing c.HTTPClient.CheckRedirect from httpClient() would mutate a struct shared by every request goroutine for the process's lifetime — a data race, and the exact hazard the same review's next point warns about. The copy shares the Transport, which is safe for concurrent use by design. **Two comments described things that were not true.** The manager's doc claimed reading a stored credential "is permission-checked as one"; there is no per-record Read check, just the single Create check, which covers it because Create is stronger and the lookup is account-scoped. Said that instead. The modelDiscovery field now records that it is shared across requests and must stay read-only after construction. **The handler test asserted neither the label contract nor the upstream.** The response omits label entirely when a vendor supplies none, and the dashboard falls back to the id on absence — an empty string would render a blank row. The fixture had no label-less model to prove it with (the review described one, but both existing entries carry labels), so this adds one. The upstream assertion matters because Bedrock's region is read back out of it. Also fixes the "blank catalog provide" subtest name.