-
Notifications
You must be signed in to change notification settings - Fork 283
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
test(connector-corda): fix flaky corda-v5-flow.test.ts #3322
test(connector-corda): fix flaky corda-v5-flow.test.ts #3322
Conversation
Fixes hyperledger-cacti#2978 Signed-off-by: adrianbatuto <[email protected]>
Depends on #3241 Please merge only after #3241has been merged as well. |
const response = await fetch(`${req.baseUrl}flow/${holdingIDShortHash}`, { | ||
method: `POST`, | ||
headers, | ||
body: cordaReqBuff, | ||
agent, | ||
}); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
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.
@petermetz the URL's are the input corresponding to the REST api calls (which will be a variable), so I think its a false positive CodeQL review point
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.
@jagpreetsinghsasan If I send in a request with baseUrl
being set to http://evil.website.with.malware.example.com
then we'll go to that link and bad things could happen.
Is there any reason why we can't pass in the hostname of the Corda node(s) in the constructor of the connector plugin? That would make it less flexible but then we only have to worry about a malicious system administrator who deployed the system with a bad configuration (an acceptable risk because if the person deploying the software is malicious, all is lost anyway)
const response = await fetch(`${req.baseUrl}cpi`, { | ||
method: `GET`, | ||
headers: headers, | ||
agent, | ||
}); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
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.
The URL's are the input corresponding to the REST api calls (which will be a variable), so I think its a false positive CodeQL review point
const response = await fetch( | ||
`${req.baseUrl}flow/${holdingIDShortHash}/${clientRequestId}`, | ||
{ | ||
method: `GET`, | ||
headers: headers, | ||
agent, | ||
}, | ||
); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
The
URL
user-provided value
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.
The URL's are the input corresponding to the REST api calls (which will be a variable), so I think its a false positive CodeQL review point
const response = await fetch(`${req.baseUrl}flow/${holdingIDShortHash}`, { | ||
method: `GET`, | ||
headers: headers, | ||
agent, | ||
}); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
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.
The URL's are the input corresponding to the REST api calls (which will be a variable), so I think its a false positive CodeQL review point
get: async () => ({ | ||
isProtected: true, | ||
requiredRoles: [], | ||
httpsAgent: new https.Agent({ rejectUnauthorized: false }), |
Check failure
Code scanning / CodeQL
Disabling certificate validation High
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.
@adrianbatuto this piece of code is no longer used, can be removed safely
@adrianbatuto Please put the declaration in the commit message and the PR description otherwise the bot doesn't see it. |
Fixes hyperledger-cacti#3293 Signed-off-by: adrianbatuto <[email protected]>
3e1660d
to
bf0f92a
Compare
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.
@adrianbatuto Please see above, same request as before.
One more in addition though: The security issues highlighted by CodeQL seem like true positives, could you please address those?
One more request: Please explain in detail why do you need the http agent property in the authorization config for the endpoint (in the commit message and the PR description)
This PR/issue depends on:
|
Closing this PR as the changes here have been included in #3241. |
Commit to be reviewed
test(connector-corda): fix flaky corda-v5-flow.test.ts
Fixes #3293
Depends on #2978
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.