Skip to content

Return the secret sentinel for remote admin config gets - #11093

Merged
thebentern merged 1 commit into
developfrom
admin-config-secret-roundtrip
Jul 20, 2026
Merged

Return the secret sentinel for remote admin config gets#11093
thebentern merged 1 commit into
developfrom
admin-config-secret-roundtrip

Conversation

@caveman99

@caveman99 caveman99 commented Jul 20, 2026

Copy link
Copy Markdown
Member

writeSecret is a setter: it swaps in the stored value only when the buffer holds the sekrit sentinel. On the NETWORK_CONFIG get path the buffer already holds the stored psk, so the call was a no-op. MQTT_CONFIG returned moduleConfig.mqtt.password verbatim.

Both get paths now return secretReserved when req.from != 0, matching the existing SECURITY_CONFIG handling. The sentinel rather than an empty string, because the set paths are wholesale struct copies with no preservation of the previous value, so a client that gets and writes back would otherwise clear the stored secret. Both set paths now call writeSecret to complete that round trip.

Tests in test_admin_session_repro: remote and local gets for wifi_psk and mqtt.password, plus a set-path round trip. handleGetModuleConfig added to AdminModuleTestShim.

20/20 on native-windows.

Summary by CodeRabbit

  • Security Enhancements
    • Sensitive WiFi and MQTT credentials are now redacted in responses to remote requests.
    • Reserved placeholder values can be used to preserve existing credentials during configuration updates.
  • Bug Fixes
    • Prevented read-modify-write updates from unintentionally clearing stored WiFi passwords.
  • Tests
    • Added coverage for local versus remote credential visibility and placeholder-based updates.

writeSecret is a setter, so calling it on the NETWORK_CONFIG get path was a no-op: the buffer
already holds the stored psk, never the sentinel. MQTT_CONFIG returned the broker password
verbatim.

Both get paths now return secretReserved when req.from != 0, and the matching set paths call
writeSecret so a read-modify-write round trip keeps the stored value.
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ad80e42-d281-455b-8dc8-824ffaaea4ed

📥 Commits

Reviewing files that changed from the base of the PR and between 0199a1f and ab0b4a3.

📒 Files selected for processing (3)
  • src/modules/AdminModule.cpp
  • test/support/AdminModuleTestShim.h
  • test/test_admin_session_repro/test_main.cpp

📝 Walkthrough

Walkthrough

Adds reserved-placeholder handling for WiFi and MQTT credentials. Configuration updates preserve stored secrets when given sekrit, while remote GET responses redact those secrets. Tests cover local versus remote responses and WiFi sentinel round-trips.

Changes

Credential protection

Layer / File(s) Summary
Secret-preserving configuration updates
src/modules/AdminModule.cpp
WiFi PSK and MQTT password updates preserve existing stored values when the incoming credential is sekrit.
Remote redaction and regression coverage
src/modules/AdminModule.cpp, test/support/AdminModuleTestShim.h, test/test_admin_session_repro/test_main.cpp
Remote network and MQTT responses return sekrit, local responses retain plaintext values, and tests cover redaction plus WiFi sentinel round-trips.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested labels: bugfix

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers the change, but it does not follow the required template or include the attestations section. Rewrite the PR description using the repository template and fill in the testing/regression attestations, including any hardware not tested.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: remote admin config gets now return the secret sentinel.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch admin-config-secret-roundtrip

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

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

⚡ Try this PR in the Web Flasher

Flash this PR in the Web Flasher

firmware commit boards expires

Warning

This is an automated, unreviewed CI test build. Back up your device configuration
before flashing, and only flash devices you are able to recover.

Supported boards built by this PR (30)
Device Board Platform
Crowpanel Adv 3.5 TFT elecrow-adv-35-tft esp32-s3
Heltec HT62 heltec-ht62-esp32c3-sx1262 esp32-c3
Heltec Mesh Node 096 heltec-mesh-node-t096 nrf52840
Heltec Mesh Node T1 heltec-mesh-node-t1 nrf52840
Heltec Mesh Node T114 heltec-mesh-node-t114 nrf52840
Heltec V3 heltec-v3 esp32-s3
Heltec V4 heltec-v4 esp32-s3
Meshnology W10 meshnology_w10 esp32-s3
Raspberry Pi Pico pico rp2040
Raspberry Pi Pico W picow rp2040
RAK WisMesh Pocket V3 rak_wismesh_pocket nrf52840
RAK WisMesh Pod rak_wismesh_pod nrf52840
RAK WisMesh Repeater Mini V2 rak_wismesh_repeater_mini nrf52840
RAK WisMesh Tag rak_wismeshtag nrf52840
RAK WisBlock 11200 rak11200 esp32
RAK WisBlock 11310 rak11310 rp2040
RAK3312 rak3312 esp32-s3
RAK WisBlock 4631 rak4631 nrf52840
Seeed SenseCAP Mesh-Tracker-X1 seeed_mesh_tracker_X1 nrf52840
Seeed Wio Tracker L1 seeed_wio_tracker_L1 nrf52840
Seeed Xiao NRF52840 Kit seeed_xiao_nrf52840_kit nrf52840
Seeed Xiao ESP32-S3 seeed-xiao-s3 esp32-s3
Station G2 station-g2 esp32-s3
Station G3 station-g3 esp32-s3
LILYGO T-Deck t-deck-tft esp32-s3
LILYGO T-Echo t-echo nrf52840
LILYGO T-Echo Plus t-echo-plus nrf52840
LILYGO T-Impulse Plus t-impulse-plus nrf52840
LilyGo T3-C6 tlora-c6 esp32-c6
Seeed SenseCAP T1000-E tracker-t1000-e nrf52840

Build artifacts expire on 2026-08-19. Updated for ab0b4a3.

@caveman99 caveman99 added the bugfix Pull request that fixes bugs label Jul 20, 2026
@thebentern
thebentern merged commit 6f522aa into develop Jul 20, 2026
105 of 107 checks passed
@thebentern
thebentern deleted the admin-config-secret-roundtrip branch July 20, 2026 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Pull request that fixes bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants