-
Notifications
You must be signed in to change notification settings - Fork 86
DOCS: Fixing security scan issue #2576
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
base: master
Are you sure you want to change the base?
Conversation
Adding in 'state' to help with CSRF a bit Some clean up to ensure the params are escaped
| const state = crypto.randomUUID() | ||
| sessionStorage.setItem(SETTINGS.storageKey, state); |
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.
This is the MAJOR update here. We need to store this off and validate it down below.
|
| const params = new URLSearchParams({ | ||
| client_id: SETTINGS.appId, | ||
| redirect_uri: SETTINGS.redirectUri, | ||
| resource_id: SETTINGS.resourceId, | ||
| response_type: SETTINGS.responseType, | ||
| [SETTINGS.stateKey]: state, | ||
| }); |
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.
Setting up the params in this way should handle escaping for us.
| if (authCode === "error") { | ||
| return ( | ||
| <span style={{ color: "red" }}> | ||
| We were unable to verify this worked. Please contact support. | ||
| </span> | ||
| ); | ||
| } |
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.
This is where things get weird. Normally in an oAuth situation we might consume this code and do something with it. However, here we are just seeing that things are setup. So I think this is the best we could do here 🤷 .
Like if this happens either their browser messed up or possibly someone messed with their stuff and they just setup something wrong.
mdibaiee
left a comment
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, let me know if you need help testing this
https://github.com/estuary/flow/security/code-scanning/8
Description:
Escaping the
tenantnameWorkflow steps:
This will impact how Azure gets setup. This still needs tested
Documentation links affected:
Notes for reviewers:
(anything that might help someone review this PR)