-
Notifications
You must be signed in to change notification settings - Fork 45
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
FORMS-1563: auto-approve new ext api when same ministry/url already a… #1543
base: main
Are you sure you want to change the base?
FORMS-1563: auto-approve new ext api when same ministry/url already a… #1543
Conversation
…pproved. Signed-off-by: Jason Sherman <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
Removed 4 "dead" deployments with your help, and have deployed this one. Best way to test is 2 forms with different ministries then add some multiple External APIs with the same base URL to each form and do approvals in the Admin/APIs section. All matching External APIs should be approved after first Admin approval. Then back in the form, add another API with the same base url and it should be auto-approved. |
Signed-off-by: Jason Sherman <[email protected]>
Release e8324f9 deployed at https://chefs-dev.apps.silver.devops.gov.bc.ca/pr-1543 |
I don't know if this auto approval is possible. If I was to find out an URL for another ministry's external API, then I could create a form misrepresented as being from that ministry, and with a matching external API. Then I just wait for their external API to get approved and I could then start sending data to their URL from CHEFS. |
app/src/forms/admin/service.js
Outdated
const baseUrl = data.endpointUrl.split(delimiter)[0]; | ||
await ExternalAPI.query(trx) | ||
.patch({ code: ExternalAPIStatuses.APPROVED }) | ||
.whereRaw(`"formId" in (select id from form where ministry = '${adminExternalAPI.ministry}')`) |
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.
There is an SQL injection possible with this raw query. It's convoluted, but unknown if/how a bigger problem could be made:
- Create a form "A"
- Copy your bearer token and curl in a put request for the form data and set the ministry to
' OR 'x'='x
, so that the where clause evaluates to true no matter what the ministry is - Create an external API for "A"
- Create a form "B" in a different ministry and give it the same URL for an external API
- Approve form A's external API, and form B's external API will be automatically approved.
Hopefully the admin will notice this:
but perhaps there are ways to obscure it.
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.
That would be a bug in form API. And would be a db issue that we have no integrity on that column. So I can "block" that here but that's not where the bugs are. Even worse, Ministry values are frontend only. See: #1188
Adding a bandaid to prevent sql injection in this query, but bugs should be filed and addressed.
Signed-off-by: Jason Sherman <[email protected]>
External APIs are read-only and should be protected, either with an API Key or by examining the incoming headers (who the user is and from which form) or both. There are no APPROVED external apis that are not protected with an API Key. If we only (manually) approve secured APIs then this isn't an issue. And if we (manually) approve insecure APIs, there could be a reason for it (ie. it is a public 3rd party API) and again, isn't an issue. |
Signed-off-by: Jason Sherman <[email protected]>
…api-preapprove' into feat/FORMS-1563-extapi-preapprove
Added "sendApiKey" to the Admin approval screen. Admins can be assured that any other External API configs will not work without providing a key from the API owner. Or they can get assurance that it is a public API with no restrictions. |
…pproved.
Description
Simplify Approving External APIs.
If a Ministry and an Endpoint URL (everything except query string) have already been approved, then new External API configurations for the same Ministry and Endpoint URL can be auto-approved.
On the Admin side, if you approve a Ministry/Endpoint URL for an External API configuration, then update all similar configurations (same Ministry, same endpoint).
This just makes life easier when the same owner/designer creates numerous forms or numerous External API configurations (or both!) that point at the same base API. This will also speed things up for the form owner/designer as they don't have to seek approval and wait.
Type of Change
feat (a new feature)
Checklist
Further comments