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

Allow for relative OAuth token URLs #319

Open
codedust opened this issue Aug 20, 2021 · 8 comments · May be fixed by #320
Open

Allow for relative OAuth token URLs #319

codedust opened this issue Aug 20, 2021 · 8 comments · May be fixed by #320

Comments

@codedust
Copy link
Contributor

securitySchemes-oauth-http enforces https://. However relative urls (such as tokenURL: /token) should also be allowed.

JonasGroeger added a commit to codecentric/api-oas-checker that referenced this issue Aug 23, 2021
…mes-oauth-http

Allows now `https://` (as before) well as relative urls like `/token` (new).
Relative URLs must start with a `/`.

Fixes italia#319
@ioggstream
Copy link
Collaborator

ioggstream commented Sep 1, 2021

Hi @codedust and @JonasGroeger and thanks for filing this issue!

When the OAuth token URL is relative, is it supposed to be based on the servers url? Could you please better detail the use case?

What do you think about adding an x-sandbox flag to disable https:// checks for that given parameter?

@JonasGroeger
Copy link

When the OAuth token URL is relative, is it supposed to be based on the servers url? Could you please better detail the use case?

Precisely.

What do you think about adding an x-sandbox flag to disable https:// checks for that given parameter?

Currently there is no such feature on the https url aswell. So I think this is out of scope for now.

@ioggstream
Copy link
Collaborator

@JonasGroeger in servers you can mark an url as an x-sandbox to skip the check, eg.

servers:
  - url: /foo
    x-sandbox: true

@JonasGroeger
Copy link

JonasGroeger commented Sep 2, 2021

Hi Roberto,

Maybe I'm not understanding something here. If so, do tell me. Relative URLs are explicitly allowed in OpenAPI 3.x. The spec says:

Unless specified otherwise, all properties that are URLs MAY be relative references as defined by RFC3986. Unless specified otherwise, relative references are resolved using the URLs defined in the Server Object as Base URL. Note that these themselves MAY be relative to the referring document.

tokenUrl: The token URL to be used for this flow. This MUST be in the form of a URL. [...]

@ioggstream
Copy link
Collaborator

@JonasGroeger the goal of oas-checker rules is to provide additional checks respect to the ones included in openapi. In this respect, people using the security ruleset wants to be sure that eg. OAuth2 endpoints are https protected.

So, using relative URLs is perfecly ok for OAS, but when you use the additional "extra security check" ruleset, it complains because it can't be sure that this relative URL is https-protected.

The easy solution is to skip the check in some way (eg. x-noqa, ... ) at the level of the check.

WDYT?

@JonasGroeger
Copy link

The correct solution would be to check recursively if the url is HTTPS I guess. I'll see if I can somehow implement that

@ioggstream
Copy link
Collaborator

ioggstream commented Sep 3, 2021

Another - simpler - idea could be to selectively disable the rule. Do you use the online validator or the spectral CLI?

The online validator could support a way to selectively disable some rules (eg. flagging them).
About creating custom ruleset where some rules are skipped, it could be interesting to document starting from spectral docs but there are probably some changes in spectral 6.

@ioggstream
Copy link
Collaborator

ioggstream commented Sep 22, 2022

We now have a javascript security rule. Can this be tweaked/duplicated to accommodate this use case? @JonasGroeger @codedust https://github.com/italia/api-oas-checker/blob/master/security/functions/checkSecurity.js

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

Successfully merging a pull request may close this issue.

3 participants