-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Open API - security update #14868
Open API - security update #14868
Conversation
…urity works the way we expect, do not redirect API requests.
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - great tests
@@ -833,7 +831,8 @@ | |||
"type": "string", | |||
"enum": [ | |||
"static", | |||
"dynamic" | |||
"dynamic", | |||
"ai" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update the description
field here as well.
Description
@shogunpurple noticed there was an issue with using the public API to access development apps from non-builder roles, when this occurred the response was a re-direct as we expect in the browser, however this makes little sense in an API call.
I've updated the test cases a bit around the public API to make this a testable scenario, as well as updating
currentapp.ts
to 403 in these cases rather than a re-direct, by detecting it is using an API key.I've also removed the
isTest
which was hiding this from our test cases and instead check if the call is coming from a browser.Final small update is the OpenAPI spec was a little hard to use as you had to work out all the variables you needed, I've defaulted these so that when imported to tools that support OpenAPI specifications it shows all the variables listed with descriptions, making it a lot quicker to get up and running.