fix(time): stabilize Date header parsing for direct clock sync#85
fix(time): stabilize Date header parsing for direct clock sync#85IRONICBo wants to merge 1 commit intomemovai:mainfrom
Conversation
📝 WalkthroughWalkthroughThe PR refactors the time fetching mechanism in the tool by replacing direct Date header extraction with a context-driven capture approach. It introduces a new handler and struct to capture HTTP Date headers, changes the fetch endpoint from Telegram API to Apple's captive portal, and updates validation logic to use the captured date value instead of direct header access. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main/tools/tool_get_time.c (1)
138-161:⚠️ Potential issue | 🟡 Minor
captive.apple.comis an undocumented internal probe, not a public time APIApple's own developer documentation notes that "the details of these probes are not considered API", meaning the endpoint's response format, availability, and behaviour can change without notice. The previous
api.telegram.orgwas similarly unofficial for time sync, but worth calling out for tracking purposes.Additionally,
captive.apple.comis intentionally non-HTTPS, designed to allow captive portal interception. This means itsDateheader is susceptible to spoofing by any on-path attacker. For an IoT device where system time gates TLS certificate validation, an adversary who controls a Wi-Fi AP could push an arbitrary clock value. This is an inherent limitation of HTTP-based clock sync (NTP has the same property), so it's a conscious tradeoff — just make sure it's documented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/tools/tool_get_time.c` around lines 138 - 161, This code uses an undocumented, non-HTTPS probe (captive.apple.com) to obtain a Date header via time_http_ctx_t and date_header_capture_handler and then calls parse_and_set_time; change this to a safer, documented approach by either (1) switching to a public, supported time API over HTTPS (and validate TLS certs) or (2) switching to a proper time protocol (e.g., esp-sntp) for time sync; alternatively make the URL configurable and add a clear comment/warning about the spoofing risk and DNS/probe instability so callers can opt in. Ensure changes touch the esp_http_client_config_t.url usage and any logic that assumes trust in the Date header before calling parse_and_set_time.
🧹 Nitpick comments (1)
main/tools/tool_get_time.c (1)
113-133: Optional: log a warning when the Date value is truncatedThe silent truncation at lines 126–128 means an overlong (malformed) Date header becomes an unparseable string at Line 160 with no log trail. Since any valid RFC 7231 Date value is ~29 characters, this will never fire in practice, but a one-liner would make debugging easier if it ever does.
🔧 Proposed change
if (len >= sizeof(ctx->date_value)) { + ESP_LOGW(TAG, "Date header value truncated (%zu bytes)", len); len = sizeof(ctx->date_value) - 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/tools/tool_get_time.c` around lines 113 - 133, The Date header capture silently truncates overlong values in date_header_capture_handler when copying evt->header_value into ctx->date_value; modify the handler to detect when strlen(evt->header_value) >= sizeof(ctx->date_value) and emit a warning (e.g. ESP_LOGW) that the Date header was truncated, including the actual header_key/header_value or a truncated preview for diagnostics, before performing the memcpy and null-termination so there is an explicit log trail if parsing later fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@main/tools/tool_get_time.c`:
- Around line 138-161: This code uses an undocumented, non-HTTPS probe
(captive.apple.com) to obtain a Date header via time_http_ctx_t and
date_header_capture_handler and then calls parse_and_set_time; change this to a
safer, documented approach by either (1) switching to a public, supported time
API over HTTPS (and validate TLS certs) or (2) switching to a proper time
protocol (e.g., esp-sntp) for time sync; alternatively make the URL configurable
and add a clear comment/warning about the spoofing risk and DNS/probe
instability so callers can opt in. Ensure changes touch the
esp_http_client_config_t.url usage and any logic that assumes trust in the Date
header before calling parse_and_set_time.
---
Nitpick comments:
In `@main/tools/tool_get_time.c`:
- Around line 113-133: The Date header capture silently truncates overlong
values in date_header_capture_handler when copying evt->header_value into
ctx->date_value; modify the handler to detect when strlen(evt->header_value) >=
sizeof(ctx->date_value) and emit a warning (e.g. ESP_LOGW) that the Date header
was truncated, including the actual header_key/header_value or a truncated
preview for diagnostics, before performing the memcpy and null-termination so
there is an explicit log trail if parsing later fails.
Signed-off-by: Bo <boironic@gmail.com>
|
Hi @mogenson, thanks for the fix! 🙏 It looks like the core issue (capturing the That said, your PR still has two valuable changes that
Could you rebase onto the latest |
@crispyberry this PR was authored by @IRONICBo. I'll let them respond. |
3 participants
This patch fixes
tool_get_timefailures in direct mode by switching the HTTP time source tohttp://captive.apple.com/and capturing theDateheader viaHTTP_EVENT_ON_HEADER.It also avoids using header memory after client cleanup by copying
Dateinto a local buffer before parsing. Time parsing still uses temporary UTC (TZ=UTC0)and then restores
MIMI_TIMEZONE, so both system clock and agent-facing formatted time stay consistent with local timezone settings.Summary by CodeRabbit