superfaceai / spec

The Comlink Specification. Comlink is a new interface description and integration language build self-integrating applications.
https://superface.ai/docs/comlink/specification
MIT License
11 stars 1 forks source link

fix: missing security #48

Closed Jakub-Vacek closed 1 year ago

Jakub-Vacek commented 1 year ago

This PR fixes missing security schemes for ApiKey in body, path and Digest security scheme. Also there is one TODO property name in existing ApiKey schemes is marked as required but in implementation it is optional.

Schema generated from implementation: https://github.com/superfaceai/ast-js/blob/dev/src/interfaces/providerjson/providerjson.schema.json

github-actions[bot] commented 1 year ago

Deploy preview for specification ready!

✅ Preview https://specification-1p2blamxz-superface.vercel.app

Built with commit 9026a5e96df4b3f241ee405a9acc4c9f702593db. This pull request is being automatically deployed with vercel-action

zdne commented 1 year ago

On the TODO - optional vs. required name – I wonder why it is optional in the implementation, seems like the header field should be specified, no?

Jakub-Vacek commented 1 year ago

@zdne name falls back to default name: https://github.com/superfaceai/one-sdk-js/blob/dev/src/core/interpreter/http/security/api-key/api-key.ts#L39 with value Authorization. Changing this to required would meant braking change that would brak a lot of provider.jsons :|

zdne commented 1 year ago

@Jakub-Vacek so, the correct way is that the header is required? If so let's at least keep track of it in product board.

Jakub-Vacek commented 1 year ago

Now, implementation checks if there (in provider.json) is specified name of header. If it isn't SDK will assume that header is called Authorization, if there is specific name it will use it as name of header. Implementation makes sense (imo) and I wouldn't change it (because of breaking changes). Personally I would update the spec but I understand that it is something we should not do on regular basis.

zdne commented 1 year ago

@Jakub-Vacek ok, thanks for the explanation!

Jakub-Vacek commented 1 year ago

@zdne So should I update the spec? 🤷🏻‍♂️