HTTP 500 returned for empty attachments in NPM package PUT request

Summary

When the API path PUT /api/:version/projects/:id/packages/npm/:package_name is called and the body of the request contains an attachment with a data field that is an empty string the API returns a 500 error with the message undefined method count' for nil:NilClass`.

This is because of this line1 which attempts to index the empty string to discover file size which results in the object being nil which causes the subsequent .count('=') to return the undefined method error.

Steps to reproduce

  1. Have a project where the packages feature is enabled
  2. Create a payload.json file with the following payload
payload 
{
    "_attachments": {
        "foobar-1.0.0.tgz": {
            "data": ""
        }
    },
    "_id": "foobar@1.0.0",
    "name": "foobar",
    "dist-tags": {
        "latest": "1.0.2"
    },
    "versions": {
        "1.0.0": {
            "name": "foobar",
            "version": "1.0.0",
            "dist": {
                "shasum": "675ea51efc43db9e74c8c6b354adc2f5db1d0221"
            },
            "tarball": "https://example.com/foobar.tgz",
            "_id": "foobar@1.0.0"
        }
    }
}
3. Run this curl command (with a valid token and project ID): 
curl --request PUT --header "Content-Type: application/json" --data @./payload.json --header "Authorization: Bearer your-token-here" https://gitlab.com/api/v4/projects/:project_id_here/packages/npm/foobar
  1. You should receive a 500 error

Example Project

Any project with package registry enabled.

What is the current bug behavior?

A 500 error is returned when bad data is sent to the API endpoint.

What is the expected correct behavior?

Some form of client side error (HTTP code 400 would be one option) should be raised instead of a 500. This would avoid it being counted as an error for calculations for top level error counts like SLOs and also help guide users to what they are doing wrong.

Relevant logs and/or screenshots

Example backtrace 
      "app/services/packages/npm/create_package_service.rb:84:in `block in calculated_package_file_size'",
      "lib/gitlab/utils/strong_memoize.rb:34:in `strong_memoize'",
      "app/services/packages/npm/create_package_service.rb:76:in `calculated_package_file_size'",
      "app/services/packages/npm/create_package_service.rb:104:in `file_size_exceeded?'",
      "app/services/packages/npm/create_package_service.rb:12:in `execute'",
      "lib/api/npm_project_packages.rb:67:in `block (2 levels) in <class:NpmProjectPackages>'",
      "ee/lib/gitlab/middleware/ip_restrictor.rb:14:in `block in call'",
      "ee/lib/gitlab/ip_address_state.rb:10:in `with'",
      "ee/lib/gitlab/middleware/ip_restrictor.rb:13:in `call'",
      "lib/api/api_guard.rb:222:in `call'",
      "lib/gitlab/metrics/elasticsearch_rack_middleware.rb:16:in `call'",
      "lib/gitlab/middleware/memory_report.rb:13:in `call'",
      "lib/gitlab/middleware/speedscope.rb:13:in `call'",
      "lib/gitlab/database/load_balancing/rack_middleware.rb:23:in `call'",
      "lib/gitlab/middleware/rails_queue_duration.rb:33:in `call'",
      "lib/gitlab/metrics/rack_middleware.rb:16:in `block in call'",
      "lib/gitlab/metrics/web_transaction.rb:46:in `run'",
      "lib/gitlab/metrics/rack_middleware.rb:16:in `call'",
      "lib/gitlab/jira/middleware.rb:19:in `call'",
      "lib/gitlab/middleware/go.rb:20:in `call'",
      "lib/gitlab/etag_caching/middleware.rb:21:in `call'",
      "lib/gitlab/middleware/query_analyzer.rb:11:in `block in call'",
      "lib/gitlab/database/query_analyzer.rb:37:in `within'",
      "lib/gitlab/middleware/query_analyzer.rb:11:in `call'",
      "lib/gitlab/middleware/multipart.rb:173:in `call'",
      "lib/gitlab/middleware/read_only/controller.rb:50:in `call'",
      "lib/gitlab/middleware/read_only.rb:18:in `call'",
      "lib/gitlab/middleware/same_site_cookies.rb:27:in `call'",
      "lib/gitlab/middleware/handle_malformed_strings.rb:21:in `call'",
      "lib/gitlab/middleware/basic_health_check.rb:25:in `call'",
      "lib/gitlab/middleware/handle_ip_spoof_attack_error.rb:25:in `call'",
      "lib/gitlab/middleware/request_context.rb:21:in `call'",
      "lib/gitlab/middleware/webhook_recursion_detection.rb:15:in `call'",
      "config/initializers/fix_local_cache_middleware.rb:11:in `call'",
      "lib/gitlab/middleware/compressed_json.rb:37:in `call'",
      "lib/gitlab/middleware/rack_multipart_tempfile_factory.rb:19:in `call'",
      "lib/gitlab/middleware/sidekiq_web_static.rb:20:in `call'",
      "lib/gitlab/metrics/requests_rack_middleware.rb:79:in `call'",
      "lib/gitlab/middleware/release_env.rb:13:in `call'"
Example of request parameters that results in a 500 error (slightly sanitized to protect customer privacy) 
    "params": [
      {
        "key": "name",
        "value": "zsa"
      },
      {
        "key": "versions",
        "value": "{\"1.0.0\"=>{\"name\"=>\"zsa\", \"version\"=>\"1.0.0\", \"dist\"=>{\"shasum\"=>\"675ea51efc43db9e74c8c6b354adc2f5db1d0221\", \"tarball\"=>\"*****CUSTOMER URL REDACTED*****.tgz\"}, \"_id\"=>\"zsa@1.0.0\"}}"
      },
      {
        "key": "_id",
        "value": "zsa@1.0.0"
      },
      {
        "key": "dist-tags",
        "value": "{\"latest\"=>\"1.0.2\"}"
      },
      {
        "key": "_attachments",
        "value": "{\"zsa-1.0.0.tgz\"=>{\"data\"=>\"\"}}"
      }
    ],

Output of checks

This bug happens on GitLab.com

Possible fixes

https://gitlab.com/gitlab-org/gitlab/-/blob/v15.9.3-ee/app/services/packages/npm/create_package_service.rb?ref_type=tags#L84 This is where the issue comes from. As for fixes I'm no ruby expert but some checking if the value retrieved via the data attribute is not empty string would likely do the trick followed by raising a 400 at that point if it is empty.

  1. Note that this link corresponds to the version we saw the error in but the same code exists currently in the master branch. I wanted to make sure that the line reference stayed intact in case of changes before this bug is resolved. ↩

Edited by Andy Knight