Disable ambient proxies for protected clients

[Chris0Jeky]

Summary

• Disable ambient and system proxies for the OpenAI, Gemini, Ollama, and outbound-webhook clients so DNS and IP validation remains bound to the configured direct origin.
• Preserve the existing localhost opt-ins, redirect blocking, cancellation behavior, and sanitized failures.
• Keep protected request details out of Taskdeck-configured logging, OpenTelemetry, metrics, Sentry outbound instrumentation, and pre-handler System.Net.Http EventSource path/query payloads; server-side Sentry exception tracking remains available.
• Add hostile-proxy, provider-dispatch, policy, telemetry, and documentation coverage.

Verification state

Published head: dc2a099c6c8fd5152c7f7f0e4462869054111e4f
Absorbed base: 88eb6776ffd0ccaa5a6c87a88cbba31588dc9390

Local evidence on the identical backend tree:

• affected API lane: 66 passed / 0 failed / 0 skipped
• Application provider/policy/constructor/remasking lane: 158 / 0 / 0
• provider dispatch: 30/30 across five fresh processes
• registered EventSource and scoped-Sentry controls: 6/6 across three fresh processes
• full serialized backend: 7,539 passed / 5 intentional skips / 0 failed
• docs, Golden Principles, GitHub operations, diff, and 17-commit DCO gates: passed
• independent exact-base security review and exact-final-head correctness/docs review: no findings
• closing-link audit: exactly [Security] Make SSRF origin enforcement proxy-safe across OpenAI, Gemini, Ollama, and webhooks #1513

All previous review findings are mapped to fixes and verification in the latest PR evidence comment. No hosted result from an older head is reused. Exact-head Required CI, triggered security/extended checks, and the requested Codex review are pending.

Boundary

The protected transports are direct-only: no proxy-aware mode is introduced, existing exact-localhost opt-ins remain narrow, and corporate proxy-only deployments fail closed. Independently installed process-global activity/meter listeners and transport-stage host/IP observers remain outside the documented telemetry guarantee.

Closes #1513

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Exact-head local verification

Head: d1cfcc880e86ea003eeefdbfdf27f780f38d459a

• protected-client registration/egress lane: 47 passed
• full serialized backend: 7,503 passed / 5 skipped / 0 failed
• Release build: 11 pre-existing warnings / 0 errors
• docs, Golden Principles, GitHub operations, diff, and DCO gates: passed
• closing-link audit: exactly [Security] Make SSRF origin enforcement proxy-safe across OpenAI, Gemini, Ollama, and webhooks #1513

The direct-only boundary is intentional: no proxy-aware mode, no egress-envelope/audit expansion, and corporate proxy-only networks fail closed. Existing exact localhost opt-ins are unchanged.

@codex review exact head d1cfcc8, please.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Independent correctness/configuration review

Reviewed exact head d1cfcc880e86ea003eeefdbfdf27f780f38d459a against issue #1513, the complete diff/call graph, all 3 top-level comments, 0 submitted reviews, 0 inline comments, and 0 review threads. The two Codex quota notices are informational failures to produce a review, not review evidence.

MEDIUM

1. The new “allowed direct origin” test bypasses the selected providers, so OpenAI/Gemini localhost mode remains false-green. LlmProviderSelectionPolicy.Evaluate deliberately passes allowDevelopmentLocalhostEndpoints = true for OpenAI/Gemini in a development-like environment (LlmProviderSelectionPolicy.cs:49-68). Once selected, however, every OpenAI/Gemini operation re-runs TryValidateOpenAiSettings / TryValidateGeminiSettings without that argument (OpenAiLlmProvider.cs:31,140,413,425; GeminiLlmProvider.cs:30,168,434,446), so the optional argument falls back to false and exact localhost is rejected before any request is sent. The new test at LlmProviderRegistrationTests.cs:58-66 only extracts the named handler and drives it with HttpMessageInvoker; it never resolves or calls ILlmProvider, so it cannot catch this split policy. This contradicts the newly clarified LM Studio/LocalAI localhost contract in docs/platform/LLM_PROVIDER_SETUP_GUIDE.md:41-50 and does not prove the issue AC that existing direct-origin behavior remains covered. Please make runtime revalidation consume the same effective environment/opt-in policy (without broadening literal loopback/private addresses) and add provider-level exact-localhost success coverage for OpenAI and Gemini.

LOW

2. The canonical evidence checkpoint is already stale at this exact head. docs/STATUS.md:7 and docs/IMPLEMENTATION_MASTERPLAN.md:16 say full backend verification and CodeQL remain outstanding, while the exact-head PR evidence records 7,503 passed / 5 skipped and CodeQL is green. Keep the “not shipped / reviews outstanding” boundary, but reconcile the completed gates so the source-of-truth docs do not contradict the PR's exact-head evidence.

3. Ollama is now named as a supported nested provider without documenting its configuration contract. docs/platform/CONFIGURATION_REFERENCE.md:211-230 adds Ollama to the binding/selector/transport description, but the supposedly comprehensive table stops after Gemini and omits Llm:Ollama:BaseUrl, Model, TimeoutSeconds, and AllowLocalhostEndpoints. The config-shape example in docs/platform/LLM_PROVIDER_SETUP_GUIDE.md:58-80 likewise omits the Ollama object immediately after the guide starts claiming Ollama support and its extra localhost opt-in. Please document the actual environment-sensitive defaults and the full opt-in shape.

Verification run

• dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release --no-restore -m:1 --filter 'FullyQualifiedName~LlmProviderRegistrationTests|FullyQualifiedName~OutboundWebhookConnectCallbackTests|FullyQualifiedName~OutboundWebhookDeliveryWorkerTests' — 47 passed, 0 failed, 0 skipped.
• Five repetitions of the real factory-pipeline cases (FullyQualifiedName~AddLlmProviders_Should|FullyQualifiedName~AddTaskdeckWorkers_Should, --no-build) — 100/100 passed total. I found no proxy-invocation race or listener/test-isolation defect.
• node scripts/check-docs-governance.mjs — passed.
• node scripts/check-golden-principles.mjs — passed.
• git diff --check origin/main...HEAD — passed.

The four scoped handler registrations themselves consistently set UseProxy = false, retain the connect callback, and keep redirects disabled. At the time of this review, exact-head Required CI had one E2E Smoke job still in progress; this comment is a correctness review, not a merge verdict.

[Chris0Jeky]

Independent adversarial security / SSRF transport review

Reviewed exact head d1cfcc880e86ea003eeefdbfdf27f780f38d459a against main at c3f9abaa9f95870b10edd8a81b922c7af6f66986, issue #1513, the full 10-file diff, all four protected registration and send paths, all four existing top-level comments, 0 submitted reviews, 0 inline comments, and 0 review threads.

Verdict: changes required — 1 HIGH security finding. The proxy/SSRF transport change itself is sound: all four real factory pipelines terminate in the intended SocketsHttpHandler, set UseProxy = false, retain ConnectCallback, and disable automatic redirects. I found no repo-level ConfigureHttpClientDefaults, IHttpMessageHandlerBuilderFilter, later primary-handler replacement, or alternate production construction path. The .NET 8 pool manager resolves neither a custom proxy nor HttpClient.DefaultProxy unless _useProxy is true, so this single gate covers HTTP_PROXY / HTTPS_PROXY / ALL_PROXY, Windows/system proxy implementations, PAC/multi-proxy selection, default proxy credentials, and an explicitly assigned IWebProxy (runtime source). Handler pooling does not weaken that: each factory rotation rebuilds the same immutable primary-handler settings.

CRITICAL

None.

HIGH

1. Protected URI/query and credential markers are logged before the connect-time SSRF rejection.

   • Paths: backend/src/Taskdeck.Api/Extensions/LlmProviderRegistration.cs:85-151; backend/src/Taskdeck.Api/Extensions/WorkerRegistration.cs:24-41; backend/tests/Taskdeck.Api.Tests/OutboundWebhookConnectCallbackTests.cs:266-290; backend/src/Taskdeck.Api/Program.cs:260-267; backend/src/Taskdeck.Api/appsettings.json:7-11; policy conflict at docs/security/SECURITY_LOGGING_REDACTION.md:14-18.
   • The registrations use AddHttpClient without suppressing or replacing its default logging handlers. Production installs console/debug providers and has Logging:LogLevel:Default = Information.
   • The new canary deliberately puts SensitiveMarker in the query, Authorization header, and body at test lines 277-281, but only checks the thrown exception and proxy invocation count at lines 283-290. services.AddLogging() has no capturing provider, so the test cannot observe what the real factory logging handlers emit.
   • .NET 8 automatically decorates each named client with logical/client logging handlers (builder filter). Both run before the primary handler. At Information they format request.RequestUri.AbsoluteUri, including the complete query (client handler, logical handler). At Trace they log request-header values, and the default ShouldRedactHeaderValue returns false (factory options). Thus the query marker leaks at the shipped Information level even though ConnectCallback subsequently rejects the destination; if an operator enables Trace, OpenAI bearer keys, Gemini x-goog-api-key, webhook signatures, and attribution headers are also exposed. Webhook query tokens are accepted by OutboundWebhookService.cs:221-262, so this is not merely a synthetic URI shape.
   • Reproduction: attach an in-memory provider at Information/Trace (or run the normal host), resolve any of the four named pipelines, and send the existing test request POST http://10.0.0.1/protected?marker=must-not-reach-a-proxy with the marker in Authorization. Capture System.Net.Http.HttpClient.<client>.LogicalHandler and .ClientHandler: the Information entries contain the marker in the absolute URI, and Trace entries contain the header value, before the expected HttpRequestException. The proxy can remain at invocation count 0; the leak is local logging, not network egress.
   • Required fix/test: suppress default factory logging for all four protected clients (for example, RemoveAllLoggers()) and rely on existing sanitized application logs, or install a custom protected-client logger that removes the entire query and redacts Authorization, x-goog-api-key, X-Taskdeck-Webhook-Signature, and all x-taskdeck-* values. Add a real-pipeline regression with a capturing logger at Information and Trace that asserts the query/header/body marker is absent from formatted messages, structured state, scopes, exceptions, and exported activity tags on blocked and permitted-direct paths. RedactLoggedHeaders alone is insufficient because it does not sanitize AbsoluteUri.

MEDIUM

No additional security finding. The distinct pending MEDIUM correctness finding in the earlier independent review remains unaddressed; this review neither duplicates nor invalidates it.

LOW

No additional security finding. The two distinct pending LOW documentation findings in that same review also remain unaddressed.

Verification and boundary

• dotnet test backend/tests/Taskdeck.Api.Tests/Taskdeck.Api.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~LlmProviderRegistrationTests|FullyQualifiedName~OutboundWebhookConnectCallbackTests|FullyQualifiedName~OutboundWebhookDeliveryWorkerTests" — 47 passed, 0 failed, 0 skipped.
• git diff --check c3f9abaa9f95870b10edd8a81b922c7af6f66986 d1cfcc880e86ea003eeefdbfdf27f780f38d459a — passed.
• Inventory result: 4/4 protected production registrations use explicit SocketsHttpHandler; 4/4 set UseProxy = false; 4/4 retain the origin callback and redirect denial; 0 alternate production provider constructors or primary-handler overrides found.
• Existing exact-localhost opt-ins remain narrow at the callback: only the exact localhost hostname is admitted when the corresponding environment/config policy allows it; literal loopback/private/link-local addresses remain rejected.
• Explicit/system/PAC proxy-only networks fail closed as documented. This setting cannot prevent transparent network interception, service-mesh redirection, or TLS interception by a locally trusted corporate CA; those remain outside the application-proxy guarantee.
• Live check snapshot at review time: CodeQL and the relevant Windows/Ubuntu backend/API jobs were green; one E2E Smoke job was still in progress. This is a security review, not a merge verdict.

[Chris0Jeky]

Exact-head finding resolution evidence

Head: 1862b98

All prior PR feedback was inspected before this fix batch: five original top-level comments, two Codex quota notices (classified as automation non-findings), zero submitted reviews, and zero inline review threads.

• HIGH protected HttpClient URI/header logging leak: fixed in 4d55f02 with RemoveAllLoggers on every protected client and real-pipeline blocked/permitted marker-absence coverage.
• HIGH propagated trace context and protected request tags: fixed in 4d55f02 by disabling activity-header propagation and filtering marked protected requests from Taskdeck OpenTelemetry export; control requests still export.
• MEDIUM selected-provider versus runtime localhost policy mismatch: fixed in 4d55f02 through shared LlmProviderRuntimePolicy consumption at selection, provider validation, and connect time, with concrete OpenAI/Gemini/Ollama probe and completion dispatch coverage.
• MEDIUM protected HttpClient metric export: fixed in 4d55f02 with a private protected meter factory plus Taskdeck OpenTelemetry meter filtering and real export controls.
• MEDIUM process-global activity-listener test race: fixed in 4d55f02 by testing the registered pipeline without a shared global listener.
• MEDIUM chunked-body false green: fixed in 4d55f02 by fully consuming Content-Length or chunked request bodies, including the terminating chunk and trailers, before the loopback response.
• LOW documentation, Ollama Production default, concrete dispatch assertions, and injected-policy proof gaps: fixed by 4d55f02 and f4bb20d.

Local exact-head evidence: affected API lane 63/63; provider compatibility 151/151; provider dispatch repetitions 30/30; telemetry repetitions 6/6; docs governance, Golden Principles, GitHub operations, diff, and DCO gates pass. The serialized solution passed Domain 1,636, Application 3,570, API 2,168 plus four intentional skips, CLI 100, and Architecture 20 plus one intentional skip, then exposed the pre-existing Testcontainers-before-Docker-skip blocker now tracked as #1518. That blocker is not waived.

Hosted exact-head state is not merge-ready: Ubuntu API Integration failed Presence_RapidJoinLeave_EventuallyConsistent with a server HubException while Windows passed; the exact test passes locally and an identical-head failed-job rerun is queued. #1518 must land and be absorbed before the full local/hosted/review gate is repeated.

[Chris0Jeky]

@codex review exact head 1862b98, please. The local merge gate remains closed on #1518 plus the exact-head Ubuntu API Integration investigation.

[Chris0Jeky]

Final security-boundary review: changes requested

Published PR head remains 1862b98. The findings below were reviewed against the current local candidate based on main ff1ee06; none of the unpublished fixes are claimed as PR-head evidence yet.

• HIGH on the published head: enabled Sentry installs a global HTTP factory handler that can reintroduce trace/baggage headers, URL breadcrumbs, and failed-request query capture on the four protected clients. Local signed commit efc8f94 disables that automatic outbound handler while preserving server-side Sentry tracking; local commit 8f83722 isolates the process-global hub.
• MEDIUM, merge-blocking: on net8.0, System.Net.Http EventSource emits RequestStart with full PathAndQuery before any DelegatingHandler runs. Taskdeck-configured logging/OpenTelemetry/metrics/Sentry remain suppressed, but an explicitly enabled EventListener or EventPipe collector can still observe Gemini credentials or attacker-controlled webhook query text. That violates issue [Security] Make SSRF origin enforcement proxy-safe across OpenAI, Gemini, Ollama, and webhooks #1513's literal logs-and-diagnostics acceptance boundary. A production URI-shielding fix plus real EventListener control/canary is in progress; a docs-only carve-out is not accepted.
• LOW: the enabled-Sentry regression could flush a sampled transaction through the real HTTP transport during SentrySdk.Close. Local signed commit 8f064c6 replaces it with a recording no-network transport; the focused class passes 3/3 and the affected Sentry/telemetry lane passes 136/136.

No proxy/SSRF bypass beyond the already-fixed Sentry channel was found. The four primary handlers retain UseProxy=false, redirects disabled, direct-origin connect validation, disabled propagation, and isolated meters.

The PR remains changes-requested. After the EventSource fix is complete, the branch will be pushed once, then affected/full backend checks, Required/Extended/CodeQL, two exact-head reviews, and all comment/thread checks will restart on that SHA.

[Chris0Jeky]

Exact-head finding resolution and local gate evidence

Published head: dc2a099c6c8fd5152c7f7f0e4462869054111e4f
Base absorbed: 88eb6776ffd0ccaa5a6c87a88cbba31588dc9390

All issue/PR feedback was re-inspected: 8 prior top-level comments, 0 issue comments, 0 submitted reviews, 0 inline comments, and 0 review threads before this publication. The two Codex quota notices are automation non-findings.

Finding-to-fix map

• HIGH — default IHttpClientFactory URI/header logging on protected clients: fixed by 4d55f025 with RemoveAllLoggers() on all four pipelines and real blocked/permitted marker-absence controls.
• HIGH — trace propagation / protected request tags: fixed by 4d55f025 using null activity propagation, protected activity filtering, and positive control export coverage.
• HIGH — enabled Sentry could add outbound trace/baggage, URL breadcrumbs, and failed-request capture: fixed by efc8f948, scoped to the four protected clients by d3da7e2c; 8f837221 isolates Sentry state and 8f064c69 makes the regression transport network-free.
• MEDIUM — selected-provider versus runtime localhost-policy mismatch: fixed by 4d55f025 through shared runtime policy consumption with concrete OpenAI/Gemini/Ollama probe/completion dispatch coverage.
• MEDIUM — protected HTTP metric export: fixed by 4d55f025 with an isolated meter factory and Taskdeck exporter drop view plus positive control metrics.
• MEDIUM — process-global activity-listener race and chunked-body false green: fixed by 4d55f025 with registered-pipeline testing and complete Content-Length/chunked request consumption.
• MEDIUM — System.Net.Http EventSource could emit the real PathAndQuery before a delegating handler: fixed by 9108d490; providers and webhook delivery prepare a constant masked URI before HttpClient.SendAsync, and the inner handler restores the true URI only for transport/retry, then re-masks it.
• LOW — Ollama/config/runtime-policy documentation and concrete dispatch/injected-policy proof: fixed by 4d55f025, f4bb20d7, 4fa03978.
• LOW — canonical verification command omitted seven constructor/remasking tests: fixed by 076d0b54; the documented Application filter now proves 158/158.
• LOW — final exact-base wording failed to mention the absorbed [Bug] Deduplicate column create against realtime board refresh #1522 frontend/docs base merge: fixed by dc2a099c. The backend subtree is byte-identical before/after that merge (beda83ca1596fb24f0b1e21c5fe4accb4ab55b1b).

Verification

• affected API lane: 66 passed / 0 failed / 0 skipped
• documented Application provider/policy/constructor/remasking lane: 158 / 0 / 0
• provider dispatch: 30/30 across five fresh processes
• registered EventSource + scoped-Sentry controls: 6/6 across three fresh processes
• full serialized backend on the identical backend tree: 7,539 passed / 5 intentional skips / 0 failed
  • Domain 1,636; Application 3,577; API 2,171 + 4 skips; CLI 100; Architecture 20 + 1 skip; Integration 35
• docs governance, Golden Principles, GitHub-operations governance, git diff --check: passed at final head
• DCO: 17/17 PR-range commits contain Signed-off-by
• closing-link audit: exactly [Security] Make SSRF origin enforcement proxy-safe across OpenAI, Gemini, Ollama, and webhooks #1513
• independent exact-base security review: no findings
• exact-final-head correctness/docs review: no findings

No hosted result from the former 1862b985 head is reused. Required CI, Extended/CodeQL as triggered, and fresh exact-head bot/comment/thread inspection remain pending.

[Chris0Jeky]

@codex review exact head dc2a099c6c8fd5152c7f7f0e4462869054111e4f, please. All prior findings are mapped in the preceding comment; hosted gates and this fresh exact-head review are the remaining merge gates.

[Chris0Jeky]

Independent exact-head review record

Two fresh read-only reviewers inspected the published candidate and all existing PR/issue feedback. Their verdicts are recorded here because the local reviewers could not post directly.

Security / SSRF / telemetry review

Reviewed the exact-base candidate f07bb4d5324ed71c99b665b5fe703eb9538873d5 against origin/main 88eb6776ffd0ccaa5a6c87a88cbba31588dc9390.

Verdict: no CRITICAL, HIGH, MEDIUM, or LOW findings.

• the backend subtree is identical to the full-tested/reviewed tree: beda83ca1596fb24f0b1e21c5fe4accb4ab55b1b
• the aggregate backend patch is unchanged across the base merge
• all protected send paths prepare before HttpClient.SendAsync; all four handlers retain UseProxy=false, redirect denial, origin ConnectCallback, null propagation, protected metrics, and default-logger suppression
• the Sentry filter remains scoped to the four protected clients
• the current-main merge contributes no backend file to the PR diff and preserved both canonical-doc checkpoints

The final published head dc2a099c differs only by the reviewed TESTING_GUIDE wording correction below, so the security diff and verdict are unchanged.

Correctness / configuration / docs review

Reviewed final exact head dc2a099c6c8fd5152c7f7f0e4462869054111e4f.

Verdict: no CRITICAL, HIGH, MEDIUM, or LOW findings.

The reviewer found and rechecked two LOW documentation-evidence defects during the final round:

1. The Application command omitted seven constructor/remasking tests. Fixed by 076d0b54; the documented command passes 158/158.
2. The exact-base wording called the docs fix the only later local change despite the absorbed current-main frontend/docs slice. Fixed by dc2a099c; it now says “issue-scope change,” names the separate base slice, and records the identical backend subtree.

Final re-review confirmed the 7,539-passed / 5-skipped arithmetic, DCO, governance, closing-link boundary, and no hosted-current overclaim.

Residual boundary

Caller-owned public provider clients intentionally retain ordinary HttpClient behavior. Independently installed process-global activity/meter listeners, transparent interception, and transport-stage host/IP observation remain outside the documented guarantee. Future protected production send paths must continue invoking PrepareForSend.

At publication: PR top-level comments were fully inspected; issue comments 0, submitted reviews 0, inline comments 0, review threads 0. Hosted exact-head checks and the requested Codex bot review remain pending.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: dc2a099c6c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".