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

fix: Append all credentials for OpenAPI security infos #661

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

johnrengelman
Copy link
Contributor

When using an OpenAPI definition with multiple security infos that should be AND'd together (per https://docs.gptscript.ai/tools/openapi#1-security-schemes), the tools credential injection with only include 1 of the environment variables instead of all of them. The result is non-deterministic on which envvar will be included to do Go's behavior with slices.
This changes the behavior to include credential injections for all schemes declared in the 1st security info block in the api definition.

@johnrengelman
Copy link
Contributor Author

Tried to add tests for this, but the current openApi loading tests don't handle tool linking and wasn't quite sure how to set that up in the test fixture. Got an error like so -

failed linking listPets at : failed resolving github.com/gptscript-ai/credential as petstore.swagger.iobearerAuth with GPTSCRIPT_PETSTORE_SWAGGER_IO_BEARERAUTH as env and "Please provide a value for the GPTSCRIPT_PETSTORE_SWAGGER_IO_BEARERAUTH environment variable" as message and "bearer token" as field from : can not load tools path= name=github.com/gptscript-ai/credential

Comment on lines 298 to 300
for _, cred := range info.GetCredentialToolStrings(operationServerURL.Hostname()) {
tool.Credentials = append(tool.Credentials, cred)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter wants to change it to this:

Suggested change
for _, cred := range info.GetCredentialToolStrings(operationServerURL.Hostname()) {
tool.Credentials = append(tool.Credentials, cred)
}
tool.Credentials = append(tool.Credentials, info.GetCredentialToolStrings(operationServerURL.Hostname())...)

I tested it and it works the same.

@g-linville g-linville self-requested a review August 14, 2024 18:56
@g-linville
Copy link
Member

Hey @johnrengelman , thanks for the contribution. Sorry it took me so long to get to this. It looks good to me but the linter is failing. If you could apply the suggestion I left, then I can merge this. Thanks!

@johnrengelman
Copy link
Contributor Author

Pushed this change from the linter.
I tried to look into the openai_revamp implementation to see if this same problem exists, but I wasn't able to follow where the credential envvars are injected in that (the tools are built up differently).

@g-linville
Copy link
Member

Pushed this change from the linter.
I tried to look into the openai_revamp implementation to see if this same problem exists, but I wasn't able to follow where the credential envvars are injected in that (the tools are built up differently).

Yeah I think we are planning on removing that. That was mostly just an experiment, which is why it is hidden behind an env var. So no worries. Thanks again for your contribution!

@g-linville g-linville merged commit 20c983e into gptscript-ai:main Aug 16, 2024
10 checks passed
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 this pull request may close these issues.

2 participants