Two tests fail when trying to update swagger-parser dependency to address CVE-2021-3765

[feenst]

Issue

A component of the validator package is susceptible to CVE-2021-3765. This package is part of the dependency tree from hapi-swagger because of its dependency on swagger-parser 4.0.2.

`-- swagger-parser@4.0.2
  `-- z-schema@3.25.1
    `-- validator@10.11.0

When trying to raise a pull request to update to a newer version of swagger-parser as a dependency of hapi-swagger, two tests are failing.

Tests fail when trying to use the next available version of swagger-parser (4.1.0). You can see from the changelog here that version adds functionality to the validator, specifically validateRequiredPropertiesExist, which is why the tests are failing.

I'm still looking into this, but hoping from some insight from others on why the responses defined for the given tests are failing with the additional validation.

Environment

npx envinfo --npmPackages '*hapi*' --binaries

Binaries:
Node: 14.8.0 - C:\Program Files\nodejs\node.EXE
Yarn: 1.22.17 - C:\Program Files\nodejs\yarn.CMD
npm: 6.14.7 - C:\Program Files\nodejs\npm.CMD

Steps to Reproduce

• Clone the hapi-swagger repository
• npm install swagger-parser@4.1.0
• npm run test

Expected Behavior

Tests succeed

Actual Behavior

Two tests fail:

array with required #249
replace example with x-example for response

Reproducible Demo

Follow steps to reproduce.

[robmcguinness]

See #514. @feenst would be great to get a PR to resolve this upgrade.

[feenst]

@robmcguinness , you mention in #514 that the changes in swagger-parser PR #83 and PR #84 don't take into account $ref.

I'm looking through the OpenAPI specification to better understand how $ref is supposed to work, but I'm still not clear.

One of the test cases that fails has a definition structured like this:

{
  datapoint: {
    properties: {
      date: {
        type: 'string',
        format: 'date'
      },
      value: {
        type: 'number'
      }
    },
    required: ['date', 'value'],
    type: 'object'
  },
  datapointlist: {
    type: 'array',
    items: {
      $ref: '#/definitions/datapoint'
    },
    required: ['datapoint']
  }
}

The validateSpec function in swagger-parser iterates through each definition and runs the validateRequiredPropertiesExist function for each definition.

An exception happens during validation of datapointlist in validateRequiredPropertiesExist.

Is the issue is that the definition is not aligning to the specification or if the validation does not handle this valid definition correctly.

Maybe @marcelstoer can also weigh in on this.

[marcelstoer]

@feenst took me a while to understand why you dragged me into this but now I got it 😄

This here looks wrong to me. datapointlist doesn't actually have any properties (of its own) and, hence, it can't list required properties either.

  datapointlist: {
    type: 'array',
    items: {
      $ref: '#/definitions/datapoint'
    },
    required: ['datapoint']
  }

[feenst]

I met with Ben Hutton and Juan Cruz Viotti this morning at JSON Schema Office Hours to describe the problem.

Ben suggested that the required keyword only applies when the type is object based on the following from here:

For each of these types, there are keywords that only apply to those types. For example, numeric types have a way of specifying a numeric range, that would not be applicable to other types.

The array type does not include required.

Juan Cruz suggested the following addition to validateRequiredPropertiesExist:

// The "required" keyword is only applicable for objects
if (Array.isArray(schema.type) && !schema.type.includes('object')) {
  return;
} else if (!Array.isArray(schema.type) && schema.type !== 'object') {
  return;
}

I'll look into raising a pull request to swagger-parser with this change.

[marcelstoer]

@feenst thanks for this confirmation! However, rather than just skipping the validation in these cases shouldn't the validation fail exactly because array types do not include required?

Also, if you prepare a PR for this, can I ask you to maybe also take APIDevTools/swagger-parser#157 into consideration?

[feenst]

@marcelstoer, I was considering whether this would be best solved by changing the validation or changing the output of hapi-swagger (or maybe both).

I have been looking into how hapi-swagger generates the swagger.json output to see whether the required attribute could be removed, but I'm still going through the code to figure out how that gets done.

I would say that based on what I've seen so far, I think the validation should possibly fail for a different reason than why it's failing now and skipping this property validation might still be correct.

In any case, I'm still reviewing and evaluating. I'll look at that issue if / when I raise a pull request to swagger-parser.

[tibbing]

Looks like z-schema has been dropped in favor of ajv as of this PR in swagger-parser. Doesn't seem to be released yet though.

[Relequestual]

However, rather than just skipping the validation in these cases shouldn't the validation fail exactly because array types do not include required? - @marcelstoer

If you expect a specific type, it needs to be specified.
JSON Schema keywords are only applicable to specific types.
required is only applied if it's an object.
This is the same with all JSON Schema keywords which have limited applicability.

[harshtarang]

Has there been any update on this? We are waiting on the new release of hapi-swagger that fixes the CVE-2021-3765 vulnerability and we were hoping this would be resolved soon

[feenst]

Has there been any update on this? We are waiting on the new release of hapi-swagger that fixes the CVE-2021-3765 vulnerability and we were hoping this would be resolved soon

I had been working on this before leaving for vacation around the holidays. There were some test-related failures on swagger-parser I was observing on my PC (even without making code changes) that I was investigating.

I was also looking into this issue in context of my pull request.

I've had other priority items come up since returning for vacation and haven't been able to revisit this.

[qaavi]

Is there anyone who tried to update swagger-ui-dist to version 4.1.3 or higher. , where this vuln got fixed.

[anthonyrivas]

@feenst Looks like this may be waiting on an PR you wrote in Swagger Parser that is waiting on a test for validation. Is there anything I can do to help move that along so we can get this vulnerability closed up?

[glennjones]

Having reviewing the thread and both the swagger/JSON schema spec it looks like hapi-swagger should only be applying "required" property to objects and not arrays. If you take this view point it would mean that the two test are incorrect and the swagger-parser is doing what it should do. Bring this up as a bug with the other maintainers to see if we can push a fix