Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upload: don't ignore BindJSON errors #999

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

alessio-perugini
Copy link
Contributor

@alessio-perugini alessio-perugini commented Oct 24, 2024

When we send an HTTP request to the upload endpoint, we include a JSON payload. This payload is then decoded into the Upload struct using the BindJSON function from the Gin framework.

However, when we have a []byte field, the expectation is that the corresponding field sent over the wire is base64 encoded with padding. If the value is not properly padded, the function returns an error.

Before this PR, we were silently ignoring this error, leading to odd behavior on the frontend:

  • A payload with a hex field containing an unpadded base64 string is sent.
  • BindJSON silently fails and sets the HTTP status to 400.
  • Despite this failure, we continue executing the handler.
  • Once the handler completes, we return the 400 error, but by that point, the upload process has already started.

This PR fixes the issue by properly checking for BindJSON errors and exiting early if any are found.

I'm not sure if the error was ignored intentionally for compatibility reasons, but I don’t think so, since this endpoint is used by the cloud editor, which behaves unpredictably when receiving a 400 error.

@alessio-perugini alessio-perugini self-assigned this Oct 24, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 20.14%. Comparing base (6f9025e) to head (f401ddc).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #999      +/-   ##
==========================================
- Coverage   20.85%   20.14%   -0.71%     
==========================================
  Files          42       42              
  Lines        2570     3221     +651     
==========================================
+ Hits          536      649     +113     
- Misses       1949     2487     +538     
  Partials       85       85              
Flag Coverage Δ
unit 20.14% <100.00%> (-0.71%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@cmaglie cmaglie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM. I did not test it.

Copy link
Contributor

@umbynos umbynos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@alessio-perugini
Copy link
Contributor Author

alessio-perugini commented Oct 24, 2024

Tested on Linux.

  • I've used the arduino:esp32:nano_nora compiling an empty sketch.
  • Copied the /upload curl request from the cloud editor
  • Removed the padding to the hex key
  • It correctly return the error
 curl -d @/tmp/a.json -v 'http://127.0.0.1:8991/upload' \                                                                                                                                                                     
  -H 'Accept: */*' \
  -H 'Accept-Language: en-US,en;q=0.5' \
  -H 'Cache-Control: no-cache' \
  -H 'Connection: keep-alive' \
  -H 'Content-Type: text/plain;charset=UTF-8' \
  -H 'DNT: 1' \
  -H 'Origin: https://app.oniudra.cc/' \
  -H 'Pragma: no-cache' \
  -H 'Sec-Fetch-Dest: empty' \
  -H 'Sec-Fetch-Mode: cors' \
  -H 'Sec-Fetch-Site: cross-site' \
  -H 'Sec-GPC: 1' \
  -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/129.0.0.0 Safari/537.36' \
  -H 'sec-ch-ua: "Brave";v="129", "Not=A?Brand";v="8", "Chromium";v="129"' \
  -H 'sec-ch-ua-mobile: ?0' \
  -H 'sec-ch-ua-platform: "Linux"'
*   Trying 127.0.0.1:8991...
* Connected to 127.0.0.1 (127.0.0.1) port 8991
> POST /upload HTTP/1.1
> Host: 127.0.0.1:8991
> Accept: */*
> Accept-Language: en-US,en;q=0.5
> Cache-Control: no-cache
> Connection: keep-alive
> Content-Type: text/plain;charset=UTF-8
> DNT: 1
> Origin: https://app.oniudra.cc/
> Pragma: no-cache
> Sec-Fetch-Dest: empty
> Sec-Fetch-Mode: cors
> Sec-Fetch-Site: cross-site
> Sec-GPC: 1
> User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/129.0.0.0 Safari/537.36
> sec-ch-ua: "Brave";v="129", "Not=A?Brand";v="8", "Chromium";v="129"
> sec-ch-ua-mobile: ?0
> sec-ch-ua-platform: "Linux"
> Content-Length: 406653
> 
* We are completely uploaded and fine
< HTTP/1.1 400 Bad Request
< Access-Control-Allow-Credentials: true
< Access-Control-Allow-Origin: https://app.oniudra.cc/
< Vary: Origin
< Date: Thu, 24 Oct 2024 11:43:43 GMT
< Content-Length: 62
< Content-Type: text/plain; charset=utf-8
< 
* Connection #0 to host 127.0.0.1 left intact
err with the payload. illegal base64 data at input byte 381416%                      
In the attachment the test payload.

a.json

@alessio-perugini alessio-perugini merged commit d36d0e1 into main Oct 24, 2024
42 checks passed
@alessio-perugini alessio-perugini deleted the upload-don-t-ignore-bind-json branch October 24, 2024 12:11
@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants