fix: URL-decode WiFi passwords before truncating to buffer size#184
Merged
Conversation
WiFi passwords with special characters were silently corrupted during captive portal setup. URL-encoded data (e.g. ! -> %21) was truncated to fit the 65-byte output buffer before decoding, so a 63-char password with special chars could expand to ~107 encoded bytes, get chopped, then decode to ~40 garbage characters causing "Wrong Password" failures. Fix: decode into an intermediate 256-byte buffer first, then truncate the decoded result to fit the output buffer. Also suppresses -Werror=stringop-truncation for newer GCC toolchains. Closes #179 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughDefers truncation until after URL-decoding when extracting form fields: encoded segments are copied into a 256-byte temporary buffer, URL-decoded, then safely truncated/copyed into the output buffer. Also adds a per-component compiler option to disable treating Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
idf_app/main/captive_portal.c (1)
80-130: Consider consolidating duplicateurl_decode()andget_form_field()utilities.Both
captive_portal.candconfig_server.cnow have identical implementations of these functions. If you prefer DRY over code locality, these could be extracted to a shared header or utility file.🤖 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 80 - 130, The url_decode and get_form_field implementations are duplicated; extract them into a single shared utility (e.g., a new utils module) and replace the local functions with declarations that call the shared implementations. Create a header exposing url_decode(char *str) and get_form_field(const char *data, const char *field, char *out, size_t out_len), move the current logic from captive_portal.c into the new source file, update captive_portal.c (and config_server.c) to include the new header and remove the local definitions, and ensure function signatures and behavior remain identical so callers need no other changes.idf_app/main/CMakeLists.txt (1)
72-72: Consider adding a tracking comment for future removal.The flag suppression is appropriate for the new truncation pattern. Since the PR notes this is temporary until ESP-IDF 6.0, consider adding an inline comment to ensure it's not forgotten:
-target_compile_options(${COMPONENT_LIB} PRIVATE -Wno-error=stringop-truncation) +# TODO: Remove after ESP-IDF 6.0 migration (see PR `#184`) +target_compile_options(${COMPONENT_LIB} PRIVATE -Wno-error=stringop-truncation)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@idf_app/main/CMakeLists.txt` at line 72, Add an inline tracking comment next to the target_compile_options(${COMPONENT_LIB} PRIVATE -Wno-error=stringop-truncation) invocation so future readers know this suppression is temporary and should be removed when upgrading to ESP-IDF 6.0; reference the COMPONENT_LIB target and the -Wno-error=stringop-truncation flag in the comment and include a TODO with a version marker (e.g., TODO: remove when ESP-IDF >= 6.0) and optionally a short reason why it was added (truncation pattern).
🤖 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 80-130: The url_decode and get_form_field implementations are
duplicated; extract them into a single shared utility (e.g., a new utils module)
and replace the local functions with declarations that call the shared
implementations. Create a header exposing url_decode(char *str) and
get_form_field(const char *data, const char *field, char *out, size_t out_len),
move the current logic from captive_portal.c into the new source file, update
captive_portal.c (and config_server.c) to include the new header and remove the
local definitions, and ensure function signatures and behavior remain identical
so callers need no other changes.
In `@idf_app/main/CMakeLists.txt`:
- Line 72: Add an inline tracking comment next to the
target_compile_options(${COMPONENT_LIB} PRIVATE -Wno-error=stringop-truncation)
invocation so future readers know this suppression is temporary and should be
removed when upgrading to ESP-IDF 6.0; reference the COMPONENT_LIB target and
the -Wno-error=stringop-truncation flag in the comment and include a TODO with a
version marker (e.g., TODO: remove when ESP-IDF >= 6.0) and optionally a short
reason why it was added (truncation pattern).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 102350eb-974c-4ea0-8298-0c3c0508df64
📒 Files selected for processing (3)
idf_app/main/CMakeLists.txtidf_app/main/captive_portal.cidf_app/main/config_server.c
Firmware PreviewFlash this PR's firmware directly in your browser: Updated: 2026-03-21T16:18:17.284Z |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
-Werror=stringop-truncationfor newer GCC (will be removed with ESP-IDF 6.0 migration, Upgrade to ESP-IDF v6.0 (stable) #183)Test plan
!@#$%^&*)Closes #179
🤖 Generated with Claude Code
Summary by CodeRabbit