OTA http client does not check all status code (400, ...) (IDFGH-5298)

[flachvr]

Is your feature request related to a problem? Please describe.

esp_https_ota_begin return ESP_OK even if the server sent an error status code ( for instance 400 : Bad Request )

Describe the solution you'd like

Add a check on the status_code != 200. To me, if the status code is != 200 it means there was a problem and we may have issue later on.

Describe alternatives you've considered

I've modified the esp_http_client.h file, more specifically the HttpStatus_Code enum with the code that was interesting to me.

typedef enum {
    /* 3xx - Redirection */
    HttpStatus_MovedPermanently  = 301,
    HttpStatus_Found             = 302,
    HttpStatus_TemporaryRedirect = 307,

    /* 4xx - Client Error */
    HttpStatus_BadRequest        = 400,
    HttpStatus_Unauthorized      = 401,
    HttpStatus_Forbidden         = 403,
    HttpStatus_NotFound          = 404,

    /* 5xx - Server Error */
    HttpStatus_InternalError     = 500
} HttpStatus_Code;

I've also modified the _http_handle_response_code function in the esp_https_ota.c file to include even more potential errors...

static esp_err_t _http_handle_response_code(esp_http_client_handle_t http_client, int status_code)
{
    esp_err_t err;
    if (status_code == HttpStatus_MovedPermanently || status_code == HttpStatus_Found || status_code == HttpStatus_TemporaryRedirect) {
        err = esp_http_client_set_redirection(http_client);
        if (err != ESP_OK) {
            ESP_LOGE(TAG, "URL redirection Failed");
            return err;
        }
    } else if (status_code == HttpStatus_Unauthorized) {
        esp_http_client_add_auth(http_client);
    } else if(status_code == HttpStatus_NotFound || status_code == HttpStatus_Forbidden) {
        ESP_LOGE(TAG, "File not found(%d)", status_code);
        return ESP_FAIL;
    } else if (status_code == HttpStatus_InternalError) {
        ESP_LOGE(TAG, "Server error occurred(%d)", status_code);
        return ESP_FAIL;
    } else if (status_code == HttpStatus_BadRequest) {
        ESP_LOGE(TAG, "Bad request (%d)", status_code);
        return ESP_FAIL;
    } 
    /* Client error ie. code between 400 and 500 */
    else if (status_code >= 400 && status_code < 500) {
        ESP_LOGE(TAG, "Client error (%d)", status_code);
        return ESP_FAIL
    }
    /* Server error ie. code > 500 */
    else if (status_code >= 500) {
        ESP_LOGE(TAG, "Server error (%d)", status_code);
    }
    
    char upgrade_data_buf[DEFAULT_OTA_BUF_SIZE];
    // process_again() returns true only in case of redirection.
    if (process_again(status_code)) {
        while (1) {
            /*
             *  In case of redirection, esp_http_client_read() is called
             *  to clear the response buffer of http_client.
             */
            int data_read = esp_http_client_read(http_client, upgrade_data_buf, DEFAULT_OTA_BUF_SIZE);
            if (data_read <= 0) {
                return ESP_OK;
            }
        }
    }
    return ESP_OK;
}

[Alvin1Zhang]

Thanks for raising the feature request.

[AxelLin]

Just to remind v4.3-rc also needs this fix.

[AxelLin]

This fix is still not in recent update of v4.3.

[mahavirj]

@AxelLin I have added internal label for backporting this to v4.3 branch. Fix shall appear in v4.3.1 release.