fix: URL-decode WiFi passwords before truncating to buffer size

[muness]

Summary

• Fixes WiFi passwords with special characters being silently corrupted during captive portal setup
• URL-encoded data was truncated to 65 bytes before decoding — a 63-char password with special chars expands to ~107 encoded bytes, gets chopped, decodes to ~40 garbage chars → "Wrong Password"
• Fix: decode into intermediate 256-byte buffer first, then truncate the decoded result
• Minimal change: only the 3 files with get_form_field() — no other refactoring

Replaces #180 (which bundled an unrelated strncpy→rk_strlcpy refactor that broke bridge communication).

Test plan

• Flash firmware and configure WiFi with a short, simple password — should still work
• Configure WiFi with a 63-character password containing special characters (!@#$%^&*) — should now connect
• Verify config server /wifi-add POST endpoint also works with long passwords

Closes #179

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • More reliable handling of URL-decoded form inputs in captive portal and config server to prevent truncated or corrupted values.
  • Safer, deterministic handling of Wi‑Fi credentials and bridge settings so submitted SSID/password values are stored and terminated correctly, reducing risk of misconfigured connections.

[coderabbitai]

Warning

Rate limit exceeded

@muness has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 57 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cbd5aee1-5987-416d-80f1-3c58cf0d6ef5

📥 Commits

Reviewing files that changed from the base of the PR and between 002b5c1 and 9a0408e.

📒 Files selected for processing (2)

• frame_app/main/CMakeLists.txt
• idf_app/main/CMakeLists.txt

📝 Walkthrough

Walkthrough

get_form_field() in multiple apps now copies the URL-encoded substring into a fixed temporary buffer, URL-decodes there, then truncates/copies the decoded text into the caller buffer. Several places switched from strncpy(..., size-1) to explicit length-capped memcpy + manual NUL termination for credential assignments.

Changes

Cohort / File(s)	Summary
Captive portal parsing (frame) frame_app/main/captive_portal.c	get_form_field() now copies the extracted URL-encoded substring into a fixed temporary buffer, decodes there, then copies/truncates the decoded result into the caller buffer (prevents truncation-before-decode corruption).
Captive portal parsing (idf) idf_app/main/captive_portal.c	Same get_form_field() change as above; configure_get_handler() now uses memcpy with explicit size handling when copying ssid/pass into cfg.
Config server parsing idf_app/main/config_server.c	get_form_field() updated to decode in a temporary buffer then copy decoded output; config_post_handler() changed strncpy usage to explicit memcpy with length cap + NUL termination for bridge/related fields.
WiFi config helper common/rk_cfg.h	rk_cfg_add_wifi() switched SSID/password copies from strncpy to length-capped memcpy plus explicit NUL termination for both updates and new entries (password updates now conditional and correctly terminated).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nibble bytes both small and wide,

Encoded tangles once did hide.
Now I stash them, decode with care,
Then copy out the truth laid bare.
Hoppity-hop—no chars lost there! 🥕

🚥 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 clearly and concisely summarizes the main fix: URL-decoding WiFi passwords before truncating to buffer size, which matches the core change across all modified files.
Linked Issues check	✅ Passed	All objectives from issue #179 are met: intermediate buffer used for URL-encoded data (256 bytes), decoding occurs before truncation, fix applied to all three affected files, and buffer sizes preserved.
Out of Scope Changes check	✅ Passed	The strncpy-to-memcpy changes address GCC compiler warnings cited in the commit message and align with the fix's goal of deterministic byte-copy semantics; no unrelated changes detected.
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/wifi-password-url-decode-v2

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

idf_app/main/config_server.c (1)

95-100: Prefer sizing the temporary encoded buffer from max decoded field size (not a magic 256).

For this file, get_form_field() is used with buffers up to 129 bytes (bridge). A fixed 256-byte encoded buffer is not a full 3x worst-case envelope for that decoded size, so pre-decode truncation risk can reappear.

Suggested fix

-    char encoded[256];
+    // 3x expansion worst case for percent-encoding + NUL
+    char encoded[129 * 3 + 1];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@idf_app/main/config_server.c` around lines 95 - 100, The temporary encoded
buffer is a fixed 256 bytes which can be too small for URL-encoded forms when
the decoded field (e.g. buffers used by get_form_field() such as bridge)
approaches its maximum; change the encoded buffer sizing to derive from the
incoming decoded length instead of the magic 256—allocate (or ensure)
encoded_size = decoded_len * 3 + 1 (or dynamically allocate that size) and then
use that encoded_size in the subsequent checks that currently reference
encoded/sizeof(encoded); update references in get_form_field() to use the
computed encoded buffer and keep the existing truncation logic that sets len =
encoded_size - 1 when needed so pre-decode truncation cannot reoccur.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@frame_app/main/captive_portal.c`:
- Around line 178-183: The truncation math can underflow when out_len == 0;
before computing decoded_len = out_len - 1 or doing memcpy/out[decoded_len] =
'\0', add a guard that checks if out_len == 0 and handle it (e.g., return 0 or
set decoded_len = 0 and skip copies/writes); update the block that manipulates
decoded_len, memcpy(out, encoded, decoded_len) and out[decoded_len] = '\0' to
only perform the subtraction, copy and null-terminate when out_len > 0 to avoid
underflow and invalid writes (refer to variables decoded_len, out_len, encoded,
out in this function).

---

Nitpick comments:
In `@idf_app/main/config_server.c`:
- Around line 95-100: The temporary encoded buffer is a fixed 256 bytes which
can be too small for URL-encoded forms when the decoded field (e.g. buffers used
by get_form_field() such as bridge) approaches its maximum; change the encoded
buffer sizing to derive from the incoming decoded length instead of the magic
256—allocate (or ensure) encoded_size = decoded_len * 3 + 1 (or dynamically
allocate that size) and then use that encoded_size in the subsequent checks that
currently reference encoded/sizeof(encoded); update references in
get_form_field() to use the computed encoded buffer and keep the existing
truncation logic that sets len = encoded_size - 1 when needed so pre-decode
truncation cannot reoccur.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3671a5e4-37ba-4e9f-89a5-5f9ec51068d8

📥 Commits

Reviewing files that changed from the base of the PR and between b639f5b and 03b04c1.

📒 Files selected for processing (3)

• frame_app/main/captive_portal.c
• idf_app/main/captive_portal.c
• idf_app/main/config_server.c

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Guard zero-length destination buffers before truncation math.

At Line 179, decoded_len = out_len - 1 underflows when out_len == 0, which can turn this into an invalid large copy/write path.

Suggested fix

+  if (out_len == 0) {
+    return false;
+  }
   size_t decoded_len = strlen(encoded);
   if (decoded_len >= out_len) {
     decoded_len = out_len - 1;
   }
   memcpy(out, encoded, decoded_len);
   out[decoded_len] = '\0';

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@frame_app/main/captive_portal.c` around lines 178 - 183, The truncation math
can underflow when out_len == 0; before computing decoded_len = out_len - 1 or
doing memcpy/out[decoded_len] = '\0', add a guard that checks if out_len == 0
and handle it (e.g., return 0 or set decoded_len = 0 and skip copies/writes);
update the block that manipulates decoded_len, memcpy(out, encoded, decoded_len)
and out[decoded_len] = '\0' to only perform the subtraction, copy and
null-terminate when out_len > 0 to avoid underflow and invalid writes (refer to
variables decoded_len, out_len, encoded, out in this function).

[coderabbitai]

🧹 Nitpick comments (1)

idf_app/main/captive_portal.c (1)

130-147: LGTM - correctly fixes the URL-decoding truncation issue.

The new approach properly decodes into the 256-byte intermediate buffer first, then truncates the decoded result to fit the output buffer. This prevents percent-encoded sequences from being split during truncation.

One minor defensive consideration: if out_len were ever 0, line 143 would underflow (out_len - 1 wraps to SIZE_MAX). Current callers always pass valid sizes (33 and 65), so this isn't exploitable today, but a guard could prevent future misuse.

🛡️ Optional defensive check

 static bool get_form_field(const char *data, const char *field, char *out,
                            size_t out_len) {
+  if (out_len == 0) return false;
   char search[64];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@idf_app/main/captive_portal.c` around lines 130 - 147, Add a defensive guard
so out_len cannot underflow when truncated: in the function that does URL
decoding (the block using encoded[], url_decode(encoded), size_t decoded_len,
and out_len), check if out_len == 0 before computing out_len - 1 and either
return false or handle it safely; then compute decoded_len = min(decoded_len,
out_len - 1) and proceed to memcpy/out[decoded_len] = '\0'. This prevents the
underflow when out_len is zero while preserving the existing truncation
behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@idf_app/main/captive_portal.c`:
- Around line 130-147: Add a defensive guard so out_len cannot underflow when
truncated: in the function that does URL decoding (the block using encoded[],
url_decode(encoded), size_t decoded_len, and out_len), check if out_len == 0
before computing out_len - 1 and either return false or handle it safely; then
compute decoded_len = min(decoded_len, out_len - 1) and proceed to
memcpy/out[decoded_len] = '\0'. This prevents the underflow when out_len is zero
while preserving the existing truncation behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4dd6dd50-9862-459e-9f97-ec4ea88d7c81

📥 Commits

Reviewing files that changed from the base of the PR and between 03b04c1 and 002b5c1.

📒 Files selected for processing (4)

• common/rk_cfg.h
• frame_app/main/captive_portal.c
• idf_app/main/captive_portal.c
• idf_app/main/config_server.c

🚧 Files skipped from review as they are similar to previous changes (2)

• idf_app/main/config_server.c
• frame_app/main/captive_portal.c

[github-actions]

Firmware Preview

Flash this PR's firmware directly in your browser:
https://roon-knob.muness.com/pr/182/flash.html

Updated: 2026-03-21T13:45:20.728Z

[muness]

Superseded — fix applied directly to master after branch reorganization. Current master is now based on v2.5.0 (stable, compatible with bridge 3.3.2). The manifest UI work is on the v4-manifest-ui branch.