Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Swagger / JSON schema $ref to relative URIs not resolved #676

Open
scflode opened this issue Dec 7, 2016 · 13 comments
Open

Swagger / JSON schema $ref to relative URIs not resolved #676

scflode opened this issue Dec 7, 2016 · 13 comments

Comments

@scflode
Copy link

scflode commented Dec 7, 2016

Describe your problem

When having using a relative URI in the $ref (http://json-schema.org/latest/json-schema-core.html#rfc.section.7) field Dredd (or some lower level library) emits a warning telling that there cannot be an example message body (for instance) created. This is Swagger related.

warn: Parser warning in file 'swagger.json': (warning code 3) Unable to generate application/json example message body out of JSON Schema on line 23

When inlining the referenced file contents it works as expected.

What command line options do you use?

$ dredd swagger.json http://localhost --dry-run

What is in your dredd.yml?

No dredd.yml in use.

What's your dredd --version output?

dredd v2.2.5 (Darwin 15.6.0; x64)

Does dredd --level=debug uncover something?

No.

Can you send us failing test in a Pull Request?

Here a small sample to show the behavior (by using before mentioned CLI call):

swagger.json

{
  "swagger": "2.0",
  "info": {
    "title": "My API",
    "version": "1"
  },
  "basePath": "/",
  "consumes": [
    "application/json"
  ],
  "produces": [
    "application/json"
  ],
  "paths": {
    "/something": {
      "get": {
        "description": "Does something",
        "parameters": [
        ],
        "responses": {
          "200": {
            "description": "The response",
            "schema": {
              "type": "array",
              "items": {
                "$ref": "definitions.json#/SomeDefinition"
              }
            }
          }
        }
      }
    }
  }
}

definitions.json

{
  "SomeDefinition": {
    "type": "object",
    "properties": {
      "someField": {
        "type": "string"
      }
    },
    "required": ["someField"]
  }
}

The inline version (which is working as expected):

{
  "swagger": "2.0",
  "info": {
    "title": "My API",
    "version": "1"
  },
  "basePath": "/",
  "consumes": [
    "application/json"
  ],
  "produces": [
    "application/json"
  ],
  "paths": {
    "/something": {
      "get": {
        "description": "Does something",
        "parameters": [
        ],
        "responses": {
          "200": {
            "description": "The response",
            "schema": {
              "type": "array",
              "items": {
                "$ref": "#/definitions/SomeDefinition"
              }
            }
          }
        }
      }
    }
  },
  "definitions": {
    "SomeDefinition": {
      "type": "object",
      "properties": {
        "someField": {
          "type": "string"
        }
      },
      "required": ["someField"]
    }
  }
}
@honzajavorek
Copy link
Contributor

Hi @scflode, this is not possible as of now, because including random files like this can be a security issue. We do plan to have some sort of support for multiple files in both API Blueprint and Swagger, but there's no ETA yet. Maybe @netmilk could share some thoughts on this topic here as well.

Workaround would be to glue the pieces together by a different tool (there are surely tools like that for Swagger or JSON Schema) before you pass them to Dredd as a single document.

@scflode
Copy link
Author

scflode commented Dec 8, 2016

Hi @honzajavorek,

thanks for the quick reply. I just wasn't sure if I might be overlooking something.

Just as a note: Swagger UI supports this (as it's part of json-schema). The reasoning about the security issue I don't fully get. But I might miss something here (didn't think to hard about this yet). We basically use it to de-duplicate definitions like f.e. error messages which are used in different APIs and should be kept the same.

Currently I installed a workaround by using https://github.com/whitlockjc/json-refs to inline the external definitions to a temporary file and then using Dredd with this file.

Also this seems to be an issue in the Fury.js Swagger adapter and not in Dredd itself so this one likely can be closed.

Thanks for your time looking into it!

@honzajavorek
Copy link
Contributor

honzajavorek commented Mar 8, 2017

The security issue is described on following links:

Also, I wrote similar concerns in apiaryio/gavel.js#25 / apiaryio/gavel.js#89:

We need to make sure (means there should be tests for it) user cannot reference an external file in the $ref or that only JSON Schema files are supported when referenced.

...and:

Regarding external file refs, I think it's okay to support it in Gavel, but we really need to test that it will interpret only valid JSON Schema files, not files like /etc/passwd (would disclose sensitive data), /dev/zero (can eat all memory), etc. People run Dredd on their CIs, we need to prevent any vulnerabilities. Would you mind adding tests for this?

  • when referencing external file with valid content, Gavel reads the file and correctly validates according to the schema
  • when referencing external file with (possibly sensitive) plain text content, Gavel refuses to read the file and doesn't disclose the file's contents
  • when referencing huge or infinitely large external file (not sure how to test this on Windows very well), Gavel refuses to read the file and doesn't eat all computer memory or doesn't crash

I think we could add support for including files by $ref, but the mentioned cases need to be tested. @scflode would you be willing to work on that?

@artrunde
Copy link

artrunde commented Apr 6, 2017

+1

@jvrsantacruz
Copy link

jvrsantacruz commented Jun 6, 2017

+1

I plan to have all my entities as single json files to be used both by application validation and swagger. I'm not willing to maintain all that duplicated information.

@kylef
Copy link
Member

kylef commented Aug 25, 2017

We could probably allow remote referencing in the Swagger adapter based on an opt-in argument. That way it is down to the author on what they want to do providing they trust and understand what they are doing.

import FurySwaggerAdapter from 'fury-adapter-swagger';

const adapter = new FurySwaggerAdapter({allowExternalRef: true});

fury.use(adapter);

@honzajavorek If it was optional (opt-in), do you have some idea how you could signal that from Dredd. Perhaps via a CLI option or in the dredd.yml config.

@honzajavorek
Copy link
Contributor

Well I think in case of Dredd this should be opt-out rather than opt-in. The risk is there basically mostly in case you provide Dredd as a service for other people to use and you have no guarantee what people's inputs are going to be. When you run Dredd locally, it's your responsibility to be already aware of what the document contains. Dredd allows loading the document by URL, in which case the risk is similar to sth like

/usr/bin/ruby -e "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/master/install)"

i.e. you run something from the internet and you're vulnerable in case the document you are downloading does contain sth malicious, or in case it's not what you think it is (the URL is attacked and file replaced by attacker, or there's man in the middle serving you different content).

But that's my opinion. Would be nice to talk about this with @netmilk. Also, I think the requirement other than JSON-parseable files should not be includeable still holds.

MoritzStueber added a commit to UdSAES/simaas-api that referenced this issue Nov 29, 2018
This reverts commit 86bb3da.
It also breaks the ability to use dredd unless the OAS is dereferenced
first! Compare apiaryio/dredd#676
@honzajavorek
Copy link
Contributor

@omernave1 Thanks for filing #1555. I'll reply here with the full context.

The inability to include local files in Dredd made sense when Dredd and Dredd hooks was being ran on server, but that's not currently true anymore, at least for apiary.io. The sandboxed hooks have been removed from the codebase some time ago. Is there any reason left why Dredd shouldn't be able to include external API description files? 🤔

@honzajavorek
Copy link
Contributor

It would be nice if the OpenAPI adapters supported this as opt-in. Then Dredd can set the option as on by default and support referencing files like this by default.

If we see any corner cases, we can add an opt-out option to Dredd, which can turn it off, but I can't think of such situations as of now.

@combmag
Copy link

combmag commented Jun 18, 2020

this would be a really nice feature to support multiple files

@moltenice
Copy link

moltenice commented Jun 30, 2020

+1 for this feature

While putting the complete OpenAPI spec into a single file is probably alright for a small project, for larger applications it becomes a bit of a nightmare.

Ideally we would like to separate out certain parts of the spec into different files to keep things modular. The OpenAPI spec allows this.

We are planning to have a large API that is specified using OpenAPI v3 and separated out into multiple files using $ref (eg. JSON Schema for components).

We are not affected by the security concerns mentioned on here since we only run Dredd as part of our CI to ensure that the contract is being met by our API. Hence, we trust the OpenAPI file we pass in as well as all the other files it references.

This is a valid use case to use Dredd with multiple files and we'd like it if you could support it!

@safekeep-admin
Copy link

https://github.com/whitlockjc/json-refs Provided works well. Convert your OpenAPI Specs into a resolved json, then run it against dredd.

@EugenRakhimov
Copy link

Hi, is there any plans to include this, or is there any work-arounds to make it work?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants