oracle / oci-dotnet-sdk

Oracle Cloud Infrastructure SDK for .NET
https://cloud.oracle.com/cloud-infrastructure
Other
53 stars 21 forks source link

GetObject requires Object name be URL Encoded before submission #250

Open mr-russ opened 3 months ago

mr-russ commented 3 months ago

I'm not sure if this is desired behaviour or not. Please close if it is.

Having said that, in my view, as the parameter is ObjectName, then it should be a regular string and the API will encode it correctly.

Part of the Object Storage GetObject code does the following.

                ApiDetails apiDetails = new ApiDetails
                {
                    ServiceName = "ObjectStorage",
                    OperationName = "GetObject",
                    RequestEndpoint = $"{method.Method} {requestMessage.RequestUri}",
                    ApiReferenceLink = "https://docs.oracle.com/iaas/api/#/en/objectstorage/20160918/Object/GetObject",
                    UserAgent = this.GetUserAgent()
                };

requestMessage.RequestUri is constructed by ToHttpRequestMessage(), it does the following

updatedUri = updatedUri.Replace(Uri.EscapeUriString($"{{{httpRequestAttr.Name}}}"), HeaderUtils.FromValue(prop.GetValue(request)));

This again uses EscapeUriString which microsoft recommends against at https://learn.microsoft.com/en-us/dotnet/api/system.uri.escapeuristring?view=net-8.0 I'm not sure if that is how the path is parsed. If someone could indicate if that's the case, that would be helpful.

HeaderUtils.FromValue(prop.GetValue(request)) should also be escaped in URL format for GetObject. But are there other places where a path should not be escaped?

Can someone point me in the direction of where I should be looking to best resolve this issue so I can submit a patch. If it's an easy fix form someone else, I'm happy for them to do it as well.

mr-russ commented 3 months ago

updatedUri = updatedUri.Replace(Uri.EscapeUriString($"{{{httpRequestAttr.Name}}}"), HeaderUtils.FromValue(prop.GetValue(request))); is particularly tricky as it's a path. Should to HttpMessage be handling the escaping of all data inputs. I don't have enough coverage on all the API calls to see how that should happen.

At least in GetObject, you want to escape the #, as any user submitting a # want's it in the object name. However I don't know if other API calls accept a # and expect it to create a Fragment instead.

The Java SDK has an entire class for encoding path parameters; https://github.com/oracle/oci-java-sdk/blob/master/bmc-common/src/main/java/com/oracle/bmc/http/internal/ParamEncoder.java#L26

https://github.com/oracle/oci-java-sdk/blob/master/bmc-common/src/main/java/com/oracle/bmc/http/internal/ParamEncoder.java#L97 suggests the set of characters it encodes. Do you know why that set is chosen?

This is the information about default escaping in .NET https://stackoverflow.com/questions/575440/url-encoding-using-c-sharp/21771206#21771206

I'm not sure which is best to choose given the large scale of modules that depend on this function. And there are no tests or guidelines for what the parameter sets it will be sending.

github-anurag commented 3 months ago

Hi @mr-russ Thank you for bringing this up. We will look into this issue and get back to you after we have analyzed it.

github-anurag commented 3 months ago

FYI @mr-russ The OCI Dotnet SDK targets .NET Framework 2.0 and Microsoft does not warn against using this in its docs for it. Refer: https://learn.microsoft.com/en-us/dotnet/api/system.uri.escapeuristring?view=netstandard-2.0

mr-russ commented 3 months ago

Hi @github-anurag,

You are correct that you target .NET Standard 2.0. However that is more of a language interface compatibility layer rather than a binary compatibility or guarantee of behaviour between platforms. We have see cryptographic functions change behaviour between .NET Framework 4.7.2 and .NET6.

dotnet/runtime#31387 is where the deprecation happens and has an extensive discussion about the issue.

I've spent some time researching implications and am about to create a pull request with what I believe is a correct solution.

github-anurag commented 3 months ago

Thanks @mr-russ We will evaluate the PR once it is out.

mr-russ commented 3 months ago

@github-anurag see #251 for my proposed solution. There are documentation references there for how I came to that conclusion.

Further fixes to escaping were also made based on what I learnt from reading the JavaSDK, RFC's and all the .NET references to escaping.

github-anurag commented 3 months ago

@mr-russ The release with the fix should be out tomorrow. Thank you again for your contributions in making the OCI Dotnet SDK better for everyone.