Conversation
📝 WalkthroughWalkthroughThe PR adds a NimBLE-based BLE setup interface, wires it into app startup and setup-AP shutdown, enables Bluetooth build/config defaults, and makes NVS setters commit successful writes. ChangesBLE setup integration
Sequence Diagram(s)sequenceDiagram
participant app_main
participant setup_ble_start
participant NVS
participant NimBLE
participant ap_timeout_task
participant setup_ble_stop
app_main->>setup_ble_start: call after AP SSID generation
setup_ble_start->>NVS: load current config
setup_ble_start->>NimBLE: init host, GATT, and advertising
NimBLE-->>setup_ble_start: advertising active
ap_timeout_task->>setup_ble_stop: stop setup AP
setup_ble_stop->>NimBLE: stop and deinitialize
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main/main.c`:
- Around line 148-150: The BLE startup path in main() ignores the return value
from setup_ble_start(), so initialization failures are lost and startup
continues silently. Update the ap_enabled branch to check the result of
setup_ble_start(&GLOBAL_STATE), and when it is not ESP_OK, log the failure with
the returned status (for example using ESP_LOGE) so setup-mode issues are
visible in the field. Keep the change localized to the startup flow that calls
setup_ble_start().
In `@main/setup_ble.c`:
- Around line 467-518: The provisioning characteristics in
setup_ble_characteristics currently allow unauthenticated read/write access, so
update setup_ble_access_cb and the characteristic flags for the secret/config
fields to require encrypted or authenticated BLE access. Apply the proper BLE
security flags to the sensitive entries like SETUP_BLE_FIELD_WIFI_PASSWORD,
SETUP_BLE_FIELD_POOL_PASSWORD, SETUP_BLE_FIELD_COMMAND, and related provisioning
fields, and ensure the setup flow establishes the matching pairing/bonding
requirements before exposing them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c96e9bf3-9f95-4ee3-bd7f-08cc140ef19d
⛔ Files ignored due to path filters (1)
main/http_server/forge-os/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
main/CMakeLists.txtmain/NvsManager/src/nvs_config.cmain/main.cmain/setup_ble.cmain/setup_ble.hsdkconfig.defaults
| if (GLOBAL_STATE.SYSTEM_MODULE.ap_enabled) { | ||
| setup_ble_start(&GLOBAL_STATE); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
setup_ble_start() return value is discarded.
If BLE init fails (e.g. nimble_port_init error), setup_ble_start returns ESP_FAIL/ESP_ERR_NOT_SUPPORTED but startup proceeds silently. Logging the failure makes setup-mode issues diagnosable in the field.
Proposed fix
if (GLOBAL_STATE.SYSTEM_MODULE.ap_enabled) {
- setup_ble_start(&GLOBAL_STATE);
+ esp_err_t ble_err = setup_ble_start(&GLOBAL_STATE);
+ if (ble_err != ESP_OK) {
+ ESP_LOGW(TAG, "Setup BLE not started: %s", esp_err_to_name(ble_err));
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (GLOBAL_STATE.SYSTEM_MODULE.ap_enabled) { | |
| setup_ble_start(&GLOBAL_STATE); | |
| } | |
| if (GLOBAL_STATE.SYSTEM_MODULE.ap_enabled) { | |
| esp_err_t ble_err = setup_ble_start(&GLOBAL_STATE); | |
| if (ble_err != ESP_OK) { | |
| ESP_LOGW(TAG, "Setup BLE not started: %s", esp_err_to_name(ble_err)); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main/main.c` around lines 148 - 150, The BLE startup path in main() ignores
the return value from setup_ble_start(), so initialization failures are lost and
startup continues silently. Update the ap_enabled branch to check the result of
setup_ble_start(&GLOBAL_STATE), and when it is not ESP_OK, log the failure with
the returned status (for example using ESP_LOGE) so setup-mode issues are
visible in the field. Keep the change localized to the startup flow that calls
setup_ble_start().
| static const struct ble_gatt_chr_def setup_ble_characteristics[] = { | ||
| { | ||
| .uuid = &setup_ble_wifi_ssid_uuid.u, | ||
| .access_cb = setup_ble_access_cb, | ||
| .arg = (void *)SETUP_BLE_FIELD_WIFI_SSID, | ||
| .flags = BLE_GATT_CHR_F_READ | BLE_GATT_CHR_F_WRITE | BLE_GATT_CHR_F_WRITE_NO_RSP, | ||
| }, | ||
| { | ||
| .uuid = &setup_ble_wifi_password_uuid.u, | ||
| .access_cb = setup_ble_access_cb, | ||
| .arg = (void *)SETUP_BLE_FIELD_WIFI_PASSWORD, | ||
| .flags = BLE_GATT_CHR_F_READ | BLE_GATT_CHR_F_WRITE | BLE_GATT_CHR_F_WRITE_NO_RSP, | ||
| }, | ||
| { | ||
| .uuid = &setup_ble_pool_url_uuid.u, | ||
| .access_cb = setup_ble_access_cb, | ||
| .arg = (void *)SETUP_BLE_FIELD_POOL_URL, | ||
| .flags = BLE_GATT_CHR_F_READ | BLE_GATT_CHR_F_WRITE | BLE_GATT_CHR_F_WRITE_NO_RSP, | ||
| }, | ||
| { | ||
| .uuid = &setup_ble_pool_port_uuid.u, | ||
| .access_cb = setup_ble_access_cb, | ||
| .arg = (void *)SETUP_BLE_FIELD_POOL_PORT, | ||
| .flags = BLE_GATT_CHR_F_READ | BLE_GATT_CHR_F_WRITE | BLE_GATT_CHR_F_WRITE_NO_RSP, | ||
| }, | ||
| { | ||
| .uuid = &setup_ble_pool_user_uuid.u, | ||
| .access_cb = setup_ble_access_cb, | ||
| .arg = (void *)SETUP_BLE_FIELD_POOL_USER, | ||
| .flags = BLE_GATT_CHR_F_READ | BLE_GATT_CHR_F_WRITE | BLE_GATT_CHR_F_WRITE_NO_RSP, | ||
| }, | ||
| { | ||
| .uuid = &setup_ble_pool_password_uuid.u, | ||
| .access_cb = setup_ble_access_cb, | ||
| .arg = (void *)SETUP_BLE_FIELD_POOL_PASSWORD, | ||
| .flags = BLE_GATT_CHR_F_READ | BLE_GATT_CHR_F_WRITE | BLE_GATT_CHR_F_WRITE_NO_RSP, | ||
| }, | ||
| { | ||
| .uuid = &setup_ble_status_uuid.u, | ||
| .access_cb = setup_ble_access_cb, | ||
| .arg = (void *)SETUP_BLE_FIELD_STATUS, | ||
| .val_handle = &setup_ble_status_val_handle, | ||
| .flags = BLE_GATT_CHR_F_READ | BLE_GATT_CHR_F_NOTIFY, | ||
| }, | ||
| { | ||
| .uuid = &setup_ble_command_uuid.u, | ||
| .access_cb = setup_ble_access_cb, | ||
| .arg = (void *)SETUP_BLE_FIELD_COMMAND, | ||
| .flags = BLE_GATT_CHR_F_READ | BLE_GATT_CHR_F_WRITE | BLE_GATT_CHR_F_WRITE_NO_RSP, | ||
| }, | ||
| {0}, | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
ESP-IDF NimBLE GATT characteristic flags BLE_GATT_CHR_F_WRITE_ENC BLE_GATT_CHR_F_WRITE_AUTHEN secure write
💡 Result:
In ESP-IDF NimBLE, the GATT characteristic flags BLE_GATT_CHR_F_WRITE_ENC and BLE_GATT_CHR_F_WRITE_AUTHEN are used to enforce security requirements on write operations [1][2]. BLE_GATT_CHR_F_WRITE_ENC When this flag is set, the NimBLE host stack requires the BLE connection to be encrypted before a write operation is permitted [3]. If the connection is not already encrypted when a write request is received, the stack will reject the write or initiate the necessary security procedures (such as pairing/encryption) if supported [3]. Setting this flag automates the security enforcement for write access [3]. BLE_GATT_CHR_F_WRITE_AUTHEN This flag mandates that a write operation must be performed over an authenticated connection [2][4]. In Bluetooth security terms, authentication typically implies that the pairing process involved Man-in-the-Middle (MITM) protection, establishing a trusted link [5]. Similar to the encryption flag, the NimBLE stack uses this flag to check the security level of the link before allowing the write [4]. Implementation and Security Context To use these flags effectively, the device must have appropriate security manager (SMP) parameters configured [5]. These are typically set via menuconfig in the ESP-IDF project and include [5]: - I/O Capabilities: Determines the pairing method (e.g., Just Works, Passkey, Numeric Comparison). - MITM Protection: Required if you intend to use BLE_GATT_CHR_F_WRITE_AUTHEN effectively [5]. - Secure Connections (SC): It is recommended to enable LE Secure Connections for robust pairing [5]. - Bonding: Enabled if you wish to store security keys to avoid re-pairing after disconnection [5]. When configuring the ble_gatt_chr_def structure, you apply these flags in the flags field [2][6]. The NimBLE stack handles the underlying security checks in its GATT server implementation, comparing the link's current security status against the required attribute flags [7][4]. Note that these flags apply to the characteristic value; if you need to secure other operations like indications, you may need to implement additional manual checks within your access callback functions [3].
Citations:
- 1: https://sourcevu.sysprogs.com/espressif/esp-idf/files/components/bt/host/nimble/nimble/nimble/host/include/host/ble_gatt.h
- 2: https://github.com/apache/mynewt-nimble/blob/master/nimble/host/include/host/ble_gatt.h
- 3: https://github.com/espressif/esp-idf/blob/master/examples/bluetooth/ble_get_started/nimble/NimBLE_Security/README.md
- 4: https://github.com/apache/mynewt-nimble/blob/master/nimble/host/src/ble_gatts.c
- 5: https://github.com/espressif/esp-idf/blob/8c658f9f/examples/bluetooth/nimble/bleprph/tutorial/bleprph_walkthrough.md
- 6: https://mynewt.apache.org/latest/network/ble_hs/ble_gatts.html
- 7: https://sourcevu.sysprogs.com/espressif/esp-idf/files/components/bt/host/nimble/nimble/nimble/host/src/ble_gatts.c
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and search for BLE security configuration and characteristic flags.
echo "== main/setup_ble.c outline =="
ast-grep outline main/setup_ble.c --view expanded || true
echo
echo "== BLE security-related symbols in the repo =="
rg -n --hidden -S \
'WRITE_ENC|WRITE_AUTHEN|bond|pair|pairing|security|set.*security|SMP|LE Secure Connections|ENC|AUTHEN' \
main . | sed -n '1,220p'Repository: WantClue/forge-os
Length of output: 10197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== BLE-related files =="
fd -a 'setup_ble.c|ble|nimble' main components -t f || true
echo
echo "== Security / pairing configuration search =="
rg -n -S \
'ble_hs_cfg|bond|bonding|pairing|security_init|ble_gap_security|WRITE_ENC|WRITE_AUTHEN|MITM|LE Secure Connections|smp|passkey|privacy' \
main components . | sed -n '1,260p'
echo
echo "== setup_ble.c relevant lines =="
if [ -f main/setup_ble.c ]; then
nl -ba main/setup_ble.c | sed -n '1,260p'
fiRepository: WantClue/forge-os
Length of output: 804
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== main/setup_ble.c: NimBLE init and access callback =="
sed -n '520,680p' main/setup_ble.c | cat -n
echo
echo "== main/setup_ble.c: ble_hs_cfg assignments =="
rg -n -S 'ble_hs_cfg\.' main/setup_ble.c
echo
echo "== main/setup_ble.c: security-related symbols =="
rg -n -S 'WRITE_ENC|WRITE_AUTHEN|bond|bonding|pair|pairing|MITM|security|passkey|sm_io_cap|sm_bonding|sm_sc|sm_mitm' main/setup_ble.cRepository: WantClue/forge-os
Length of output: 3961
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== setup_ble_access_cb and write helpers =="
rg -n -S 'setup_ble_access_cb|write_field|SETUP_BLE_FIELD_COMMAND|SETUP_BLE_FIELD_WIFI_PASSWORD|SETUP_BLE_FIELD_WIFI_SSID|RESTART|APPLY' main/setup_ble.c
echo
echo "== surrounding code around access_cb =="
start=$(rg -n -m1 'setup_ble_access_cb' main/setup_ble.c | cut -d: -f1 || true)
if [ -n "${start:-}" ]; then
from=$((start-80))
to=$((start+220))
sed -n "${from},${to}p" main/setup_ble.c | cat -n
fiRepository: WantClue/forge-os
Length of output: 13725
Require BLE security on provisioning characteristics
These characteristics are readable and writable over an unauthenticated link, so any nearby device can read secrets (wifi_password, pool_password) and push new config or trigger APPLY/RESTART. Add encrypted/authenticated access flags plus the matching pairing/bonding setup, or gate provisioning behind a local confirmation step.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main/setup_ble.c` around lines 467 - 518, The provisioning characteristics in
setup_ble_characteristics currently allow unauthenticated read/write access, so
update setup_ble_access_cb and the characteristic flags for the secret/config
fields to require encrypted or authenticated BLE access. Apply the proper BLE
security flags to the sensitive entries like SETUP_BLE_FIELD_WIFI_PASSWORD,
SETUP_BLE_FIELD_POOL_PASSWORD, SETUP_BLE_FIELD_COMMAND, and related provisioning
fields, and ensure the setup flow establishes the matching pairing/bonding
requirements before exposing them.
1 participant
implementation of ble for setup
Summary by CodeRabbit
New Features
Bug Fixes
Chores