fix(talk): do not follow HTTP redirects on SDP POST

[SebTardif]

What Problem This Solves

Talk signaling POSTs the local SDP to the Gateway broker offerUrl with Authorization: Bearer <clientSecret>. exchange_sdp never set disable_auto_redirect on that esp_http_client. ESP-IDF follows 3xx by default and keeps request headers, so a Location hop can send the single-use broker secret (and the SDP body) off the offer host.

This PR sets disable_auto_redirect = true on that client only. A 3xx response fails the exchange (status >= 300) instead of following. Canvas image GET already chooses its own redirect policy and does not send this Bearer token.

Evidence

Unfixed exchange_sdp on upstream/main has no redirect flag next to the Bearer POST:

$ git show upstream/main:components/esp-openclaw-talk/src/esp_openclaw_talk.c | sed -n '485,496p'
    esp_http_client_config_t config = {
        .url = talk->offer_url,
        .method = HTTP_METHOD_POST,
        .timeout_ms = 30000,
        .event_handler = http_event_handler,
        .user_data = &response,
        .buffer_size = 4096,
        .buffer_size_tx = 8192,
#if CONFIG_MBEDTLS_CERTIFICATE_BUNDLE
        .crt_bundle_attach = esp_crt_bundle_attach,
#endif
    };

Host compile of the new check against that unfixed file fails (expected):

$ cc -std=c11 -Wall -Wextra -Werror components/esp-openclaw-talk/tests/test_talk_sdp_http_redirect.c -o /tmp/talk_sdp_http_redirect_test
$ /tmp/talk_sdp_http_redirect_test /tmp/talk-unfixed.c
Assertion failed: (exchange_sdp_disables_auto_redirect(src) == 1), function main, file test_talk_sdp_http_redirect.c, line 96.

Same binary against the patched source passes, and the config now pins the flag:

$ /tmp/talk_sdp_http_redirect_test components/esp-openclaw-talk/src/esp_openclaw_talk.c
talk SDP HTTP redirect tests passed
$ python3 - <<'PY'
from pathlib import Path
text = Path('components/esp-openclaw-talk/src/esp_openclaw_talk.c').read_text()
start = text.find('static int exchange_sdp(')
end = text.find('\nstatic ', start+1)
print('.disable_auto_redirect = true' in text[start:end])
PY
True

Live IDF-model hop (copy Authorization onto Location, which is what ESP-IDF does when auto-redirect stays on):

$ cc -std=c11 -Wall -Wextra -Werror /tmp/sdp-redirect-idf-demo.c -o /tmp/sdp-redirect-idf-demo
$ /tmp/sdp-redirect-idf-demo follow
offer received Authorization=yes
hop received Authorization=yes
$ /tmp/sdp-redirect-idf-demo nofollow
offer received Authorization=yes

Node fetch on the same 302 pair follows but drops Authorization (Fetch cross-origin policy). ESP-IDF does not do that drop, which is why the firmware flag is required:

$ node /tmp/sdp-redirect-demo.mjs
follow=1 status=200 offer=["Bearer broker-token"] hop=[""]
follow=0 status=302 offer=["Bearer broker-token"] hop=[]

Real behavior proof

• Behavior or issue addressed: Talk SDP POST must not follow HTTP 3xx while sending Authorization: Bearer <clientSecret>.
• Real environment tested: macOS, cc Apple clang, Node v22, worktree /tmp/oc-pr-esp-F003 on fix/talk-sdp-no-redirect from upstream/main 6f5c8e8.
• Exact steps or command run after this patch:
  cc -std=c11 -Wall -Wextra -Werror components/esp-openclaw-talk/tests/test_talk_sdp_http_redirect.c -o /tmp/talk_sdp_http_redirect_test
  then /tmp/talk_sdp_http_redirect_test on the unfixed git show dump and on the patched source
  then cc of /tmp/sdp-redirect-idf-demo.c with follow and nofollow
  then node /tmp/sdp-redirect-demo.mjs
• Evidence after fix: terminal output above. The patched exchange_sdp config contains .disable_auto_redirect = true. The host check fails on upstream/main and passes on the patched file. The IDF-model client sends the Bearer token to the hop only when following is allowed.
• Observed result after fix: 3xx is not followed. The offer host still sees the Bearer header. The hop does not. exchange_sdp already rejects status >= 300, so a 302 now ends the exchange on the offer host.
• What was not tested: flashing the ESP-IDF Unity app onto a board; a live Gateway that returns 302 on offerUrl.

Summary

exchange_sdp (Talk signaling send path) POSTs SDP with the broker clientSecret and left ESP-IDF's default redirect-follow on. That can leak the secret to a Location host. This change sets disable_auto_redirect = true on that client only.

Introduced in a1b324ad (#5, 2026-07-29). Present for 31 days.

Same-repo open PRs #28 and #31 are different findings (destroy timeout; make_parents_safely walk). No open PR already sets this flag.

Related prior art (same class: credentials must not ride a redirect hop):

• whatwg/fetch#1544 (strip Authorization on cross-origin redirect)
• curl/curl#21345 (clear credentials on redirect)
• ESP-IDF esp_http_client_config_t.disable_auto_redirect (fail closed instead of following)

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed September 7, 2026, 3:16 PM ET / 19:16 UTC.

ClawSweeper review

What this changes

The PR disables redirects for Talk’s authenticated SDP POST and adds mocked regression checks plus a source-checking CI test.

Merge readiness

⛔ Blocked before merge - 7 items remain

The hardening remains necessary on main, but this branch introduces two test-suite defects and still lacks proof through the real ESP-IDF HTTP client.

Priority: P2
Reviewed head: d4d300273aeaa41fdec168a8ef22ee39fd2b5adb
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The focused hardening is useful, but two test defects and non-production proof prevent merge readiness.
Proof confidence	🦪 silver shellfish (2/6)	Needs stronger real behavior proof before merge: Talk’s changed HTTP configuration is supported by ESP-IDF source, but the captured macOS traces exercise a source checker, a modeled client, and Node fetch—not Talk with the real ESP-IDF transport. Direct SDP success and zero requests to the redirect destination remain unobserved; the newer unavailable comment was not treated as absent proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🦐 gold shrimp (3/6)	2 actionable review findings remain.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: Talk’s changed HTTP configuration is supported by ESP-IDF source, but the captured macOS traces exercise a source checker, a modeled client, and Node fetch—not Talk with the real ESP-IDF transport. Direct SDP success and zero requests to the redirect destination remain unobserved; the newer unavailable comment was not treated as absent proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	8 items	Main still lacks the redirect restriction: The fetched main implementation initializes the bearer-authenticated POST client without disable_auto_redirect. Its response check rejects non-2xx responses only after esp_http_client_perform returns; no merged fixing PR is identified in the supplied context. Local tag inspection returned no tags, so no shipped fix is established. Production boundary and dependency signal: Talk directly includes esp_http_client.h and passes the changed configuration to esp_http_client_init. The event handler does not manually follow redirects. The component declares ESP-IDF >=5.3, and CI builds with release-v5.5; ESP-IDF is therefore the relevant HTTP contract. Redirect flag is consumed by the actual HTTP implementation: The inspected release/v5.5 implementation copies disable_auto_redirect into the client and, for redirect responses, dispatches an event instead of calling esp_http_client_set_redirection when the flag is true. This supports the production change but does not constitute after-fix execution proof. Source: https://raw.githubusercontent.com/espressif/esp-idf/release/v5.5/components/esp_http_client/esp_http_client.c.
Findings	2 actionable findings	[P2] Add the redirect field to the host HTTP configuration stub [P2] Preserve injected Gateway errors when starting signaling
Security	None	None.

How this fits together

Talk connects ESP firmware to Gateway-managed voice sessions. It receives an offer URL and single-use token from the Gateway, posts local session-description data, and passes the returned answer to WebRTC.

flowchart LR
  A[Gateway session response] --> B[Offer URL and token]
  C[Local WebRTC offer] --> D[Authenticated HTTP POST]
  B --> D
  D --> E{HTTP response}
  E -->|Redirect or error| F[Fail exchange]
  E -->|Valid SDP answer| G[WebRTC signaling]

Loading

Decision needed

Question	Recommendation
Should Talk reject every offer redirect, including same-origin redirects used by existing reverse proxies?	Require direct offer endpoints: Approve rejecting all redirects and document that operators must configure the final HTTPS Gateway endpoint before upgrading.

Why: This narrows credential forwarding but intentionally removes an existing transport behavior; source inspection cannot establish whether deployed proxies depend on it.

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: Talk’s changed HTTP configuration is supported by ESP-IDF source, but the captured macOS traces exercise a source checker, a modeled client, and Node fetch—not Talk with the real ESP-IDF transport. Direct SDP success and zero requests to the redirect destination remain unobserved; the newer unavailable comment was not treated as absent proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Add the redirect field to the host HTTP configuration stub (P2) - run_host_tests.py puts tests/host first on the include path, where esp_http_client_config_t has no disable_auto_redirect member. This initializer and the new mock's member access therefore fail compilation in the existing CI host-test command. Update the host stub alongside the production configuration.
• Preserve injected Gateway errors when starting signaling (P2) - The existing immediate-submission-failure case sets both submission errors to ESP_FAIL before calling begin_signaling. This new reset erases them, so start succeeds and the assertion at line 499 fails. Keep Gateway-state reset in the existing setup helper and initialize only the new HTTP state here.
• Resolve merge risk (P1) - Existing deployments whose offer endpoint redirects will fail Talk setup after upgrading, including same-origin redirects; the intended compatibility policy needs confirmation.
• Resolve merge risk (P1) - The captured demonstrations do not establish successful direct SDP exchange or absence of redirected requests through the real ESP-IDF client.
• Complete next step (P2) - Fix both test defects, obtain approval for rejecting all redirects, and add real ESP-IDF after-fix evidence. Terminal logs or recordings are suitable; redact tokens, addresses, and private endpoints. Update the PR body to trigger review, or ask a maintainer to comment @clawsweeper re-review.
• Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

• [P2] Add the redirect field to the host HTTP configuration stub — components/esp-openclaw-talk/src/esp_openclaw_talk.c:790
• [P2] Preserve injected Gateway errors when starting signaling — components/esp-openclaw-talk/tests/test_esp_openclaw_talk.c:202

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test growth	production +2, tests +210, workflow +5	Production growth is justified by the credential boundary; the larger test addition currently breaks the existing host suite.

Merge-risk options

Maintainer options:

1. Repair test integration (recommended)
   Add the redirect field to the host HTTP stub and preserve injected Gateway errors when beginning signaling.
2. Approve direct-endpoint requirements
   Accept redirect-dependent setup failures only with explicit policy approval, operator guidance, and real-client upgrade evidence.

Technical review

Best possible solution:

Keep credentials confined to the configured offer endpoint, with approved redirect-rejection semantics, documented proxy configuration, and real-client proof for direct success and redirected failure.

Do we have a high-confidence way to reproduce the issue?

Yes, source inspection establishes both test defects: the selected host struct lacks the new field, and begin_signaling clears preconfigured failure injection. No tests or real-client reproduction were executed during this read-only review.

Is this the best way to solve the issue?

The production flag is a narrow, supported way to prevent redirect forwarding, but the complete patch is not ready: its test integration needs repair and its compatibility contract needs approval.

Full review comments:

• [P2] Add the redirect field to the host HTTP configuration stub — components/esp-openclaw-talk/src/esp_openclaw_talk.c:790
run_host_tests.py puts tests/host first on the include path, where esp_http_client_config_t has no disable_auto_redirect member. This initializer and the new mock's member access therefore fail compilation in the existing CI host-test command. Update the host stub alongside the production configuration.
  Confidence: 0.99
• [P2] Preserve injected Gateway errors when starting signaling — components/esp-openclaw-talk/tests/test_esp_openclaw_talk.c:202
  The existing immediate-submission-failure case sets both submission errors to ESP_FAIL before calling begin_signaling. This new reset erases them, so start succeeds and the assertion at line 499 fails. Keep Gateway-state reset in the existing setup helper and initialize only the new HTTP state here.
  Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 3294af3aaa94.

Labels

Label changes:

• add merge-risk: 🚨 automation: The introduced field access and state reset break the host test compilation and an existing failure-injection case.
• add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
• remove rating: 🧂 unranked krab: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.
• remove merge-risk: 🚨 security-boundary: Current PR review merge-risk labels are merge-risk: 🚨 compatibility, merge-risk: 🚨 automation.

Label justifications:

• P2: This is a focused credential-forwarding hardening change without evidence of an ongoing exploit or widespread outage.
• merge-risk: 🚨 compatibility: Rejecting all redirects can stop existing Talk deployments that rely on redirected offer endpoints.
• merge-risk: 🚨 automation: The introduced field access and state reset break the host test compilation and an existing failure-injection case.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Talk’s changed HTTP configuration is supported by ESP-IDF source, but the captured macOS traces exercise a source checker, a modeled client, and Node fetch—not Talk with the real ESP-IDF transport. Direct SDP success and zero requests to the redirect destination remain unobserved; the newer unavailable comment was not treated as absent proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

• Main still lacks the redirect restriction: The fetched main implementation initializes the bearer-authenticated POST client without disable_auto_redirect. Its response check rejects non-2xx responses only after esp_http_client_perform returns; no merged fixing PR is identified in the supplied context. Local tag inspection returned no tags, so no shipped fix is established. (components/esp-openclaw-talk/src/esp_openclaw_talk.c:781, 3294af3aaa94)
• Production boundary and dependency signal: Talk directly includes esp_http_client.h and passes the changed configuration to esp_http_client_init. The event handler does not manually follow redirects. The component declares ESP-IDF >=5.3, and CI builds with release-v5.5; ESP-IDF is therefore the relevant HTTP contract. (components/esp-openclaw-talk/src/esp_openclaw_talk.c:790, d4d300273aea)
• Redirect flag is consumed by the actual HTTP implementation: The inspected release/v5.5 implementation copies disable_auto_redirect into the client and, for redirect responses, dispatches an event instead of calling esp_http_client_set_redirection when the flag is true. This supports the production change but does not constitute after-fix execution proof. Source: https://raw.githubusercontent.com/espressif/esp-idf/release/v5.5/components/esp_http_client/esp_http_client.c. (components/esp_http_client/esp_http_client.c:1056)
• Host configuration stub cannot compile the added field: The host esp_http_client_config_t ends at buffer_size_tx and has no disable_auto_redirect member. run_host_tests.py selects this header first and compiles the production file through the Unity translation unit; both the new initializer and mock member access consequently fail compilation. (components/esp-openclaw-talk/tests/host/esp_http_client.h:14, d4d300273aea)
• New reset erases an existing test's injected failure: The immediate-submission-failure test sets config_submit_error and create_submit_error to ESP_FAIL before calling begin_signaling. The added memset inside begin_signaling clears those values, so the call succeeds instead of returning the asserted failure. (components/esp-openclaw-talk/tests/test_esp_openclaw_talk.c:497, d4d300273aea)
• Captured proof and re-review continuity: The supplied body at sourceRevision eb6dc3ea032704e89eac91538565e58294824033e725d0c9afdcb1f702e5ab07 contains source-checker output, an IDF-model demonstration, and Node fetch output; it explicitly excludes firmware and live Gateway testing. These do not exercise Talk with the actual ESP-IDF client. The previous review requested that same final-network-effect evidence. The newer author comment could not be retrieved through either GitHub API or browser access, so its contents remain unknown.

Likely related people:

• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Repair the host configuration stub and preserve submission-error injection, then run the existing Talk host suite.
• Provide a redacted trace through Talk and the real ESP-IDF client showing direct SDP success, redirect failure, and no request reaching the redirect destination.
• Confirm and document the upgrade behavior for redirect-dependent Gateway proxies.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)

• reviewed 2026-08-30T03:34:16.720Z sha c84b574 :: needs real behavior proof before merge. :: none
• reviewed 2026-08-30T09:53:03.972Z sha c84b574 :: needs real behavior proof before merge. :: none

[SebTardif]

@clawsweeper re-review

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

• State: Complete
• Detail: The durable review result and its route handoff completed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/34155098292
• Updated: 2026-09-07T19:19:02.237Z