-
Notifications
You must be signed in to change notification settings - Fork 100
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 redirection #694
Fix redirection #694
Conversation
* init * upgrade neo * useDiagnostic * Update RpcServer.SmartContract.cs * invoke tree * add event * Add storage changes * Update nuget * rename event state * Unify * update neofs * fix json Co-authored-by: Shargon <[email protected]> Co-authored-by: Erik Zhang <[email protected]> Co-authored-by: Owen Zhang <[email protected]>
* add in file copyright * fix the copyright * update copyright start year * Delete copyright.sh * Delete copyright.txt Co-authored-by: Owen Zhang <[email protected]> Co-authored-by: Erik Zhang <[email protected]>
* init * refac Co-authored-by: Shargon <[email protected]>
@@ -62,6 +72,14 @@ public async Task<(OracleResponseCode, string)> ProcessAsync(Uri uri, Cancellati | |||
return (OracleResponseCode.Error, null); | |||
if (!Settings.Default.AllowedContentTypes.Contains(message.Content.Headers.ContentType.MediaType)) | |||
return (OracleResponseCode.ContentTypeNotSupported, null); | |||
|
|||
if (!Settings.Default.AllowPrivateHost && message.RequestMessage.RequestUri != uri) |
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.
I don't think it really works to prevent ssrf since the request is already sent in the line:
message = await client.GetAsync(uri, HttpCompletionOption.ResponseContentRead, cancellation);
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.
Yes. It will leak information such as resource existence at least because this check is not the first branch.
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.
Agree with @vang1ong7ang , I think that the redirection must be manual like #692
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.
yeah, should not assume GET
requests have no side effect.
i prefer to avoid the request happening. not only the final content uploading. |
dns rebinding should be also considered |
It seems that it's difficult to prevent SSRF in .Net. |
@dusmart also in golang i think |
Since we support https only, I think we don't need to worry about dns rebinding. |
Fix #693