fix(OTA): Wrap espota --auth flag in quotes #10126
Merged
Conversation
This file contains 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
👋 Hello per1234, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
When performing an "OTA" upload via a network port, the user may configure the sketch to require
an authentication password.
Arduino IDE presents a "Configure and Upload" dialog when the user triggers an upload to a network
port. The user can provide the authentication password via the field in that dialog. The
`upload.field.password` platform property is then set to the value provided by the user.
The platform uses the `tools.esp_ota.upload.pattern` command template to pass this value to the
espota upload tool via the tool's `--auth` flag.
Since the value of the `upload.field.password` platform property is set by the user via a free text
field, it might contain any characters. Since some characters (e.g., spaces) can be problematic in
a command line, it is essential to wrap it in quotes. This was done for the Arduino IDE 1.x variant
of the command template, but that was not ported when the pluggable discovery variant of the
command pattern was defined. This causes a spurious failure of the OTA upload process if the user
provides an authentication password that contains problematic characters.
For example:
```
"C:\Users\per\AppData\Local\Arduino15\packages\esp32\hardware\esp32\3.0.4\tools\espota.exe" \
-r -i 192.168.254.145 \
-p 3232 --auth=foo bar \
-f "C:\Users\per\AppData\Local\Temp\arduino\sketches\E2D807FABB134A2A60A1B9C7D14FE02B/11973.ino.bin"
usage: espota.exe [-h] [-i ESP_IP] [-I HOST_IP] [-p ESP_PORT] [-P HOST_PORT]
[-a AUTH] [-f FILE] [-s] [-d] [-r] [-t TIMEOUT]
espota.exe: error: unrecognized arguments: bar
Failed uploading: uploading error: exit status 2
```
This is prevented by wrapping the `--auth` flag in quotes:
```
"C:\Users\per\AppData\Local\Arduino15\packages\esp32\hardware\esp32\3.0.4\tools\espota.exe" \
a-r -i 192.168.254.145 \
-p 3232 "--auth=foo bar" \
-f "C:\Users\per\AppData\Local\Temp\arduino\sketches\E2D807FABB134A2A60A1B9C7D14FE02B/11973.ino.bin"
Sending invitation to 192.168.254.145
Authenticating...OK
Uploading: [ ] 0%
```
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
needs to be tested on all operating systems. cc @lucasssvaz |
In the PR description it is written that it was already tested in windows, linux and mac. Should I retest it ? |
Thanks for pointing that out @lucasssvaz. I added that information in after the time @me-no-dev made their comment. As I wrote in that edit, even though I happened to use output from a Windows machine in the snippets included in the content of the PR message, I did run the described test procedure on all three operating systems. On all three operating systems, I reproduced the fault (arduino/arduino-ide#2483) when using the version of the platform from before the patch proposed here, and verified that the fault does not occur (and the OTA upload works as expected) after applying the patch proposed here.
I'm sure that additional testing is always welcome. I do make a strong effort to verify that my work is valid, but I'm also a fallible human who makes plenty of mistakes. |
|
it's important to test with and without password, so a total of 3 tests per operating system:
|
|
The reason I am requesting this is the weird way that arduino builder escapes and parses the recipes on different OS. I also have a faint memory of having to deal with this when implementing OTA. Please also try with both ArduinoIDE 1.8 and ArduinoIDE 2.x/Arduino-CLI |
lucasssvaz
approved these changes
Aug 9, 2024
lucasssvaz
added
the
Area: Tools & Build System
P-R-O-C-H-Y
approved these changes
Aug 12, 2024
P-R-O-C-H-Y
added
Status: Pending Merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When performing an "OTA" upload via a network port, the user may configure the sketch to require an authentication password.
Arduino IDE presents a "Configure and Upload" dialog when the user triggers an upload to a network port. The user can provide the authentication password via the field in that dialog. The
upload.field.passwordplatform property is then set to the value provided by the user.The platform uses the
tools.esp_ota.upload.patterncommand template to pass this value to the espota upload tool via the tool's--authflag:arduino-esp32/platform.txt
Line 324 in 70786dc
Description of Change
Since the value of the
upload.field.passwordplatform property is set by the user via a free text field, it might contain any characters. Some characters (e.g., spaces) can be problematic in a command line so it is essential to wrap it in quotes. This was done for the Arduino IDE 1.x variant of the command template, but that was not ported when the pluggable discovery variant of the command pattern was defined. This causes a spurious failure of the OTA upload process if the user provides an authentication password that contains problematic characters.For example:
This is prevented by wrapping the
--authflag in quotes:Tests scenarios
arduino-esp32/libraries/ArduinoOTA/examples/BasicOTA/BasicOTA.ino
Lines 6 to 7 in 70786dc
arduino-esp32/libraries/ArduinoOTA/examples/BasicOTA/BasicOTA.ino
Line 27 in 70786dc
to:
ArduinoOTA.setPassword("foo bar");The "Configure and Upload" dialog will open.
foo barin the "Password" field of the dialog.🐛 The upload fails spuriously:
ⓘ This is done to cause the IDE to reload the updated
platform.txtfile.The "Configure and Upload" dialog will open.
foo barin the "Password" field of the dialog.🙂 The OTA upload finishes successfully.
Test performed successfully with Adafruit Feather ESP32 board on:
Related links
Originally reported at arduino/arduino-ide#2483