Skip to content
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

GetObject requires Object name be URL Encoded before submission #250

Open
mr-russ opened this issue Jul 16, 2024 · 7 comments
Open

GetObject requires Object name be URL Encoded before submission #250

mr-russ opened this issue Jul 16, 2024 · 7 comments

Comments

@mr-russ
Copy link
Contributor

mr-russ commented Jul 16, 2024

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
Copy link
Contributor Author

mr-russ commented Jul 17, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

mr-russ commented Jul 17, 2024

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
Copy link
Member

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

@mr-russ
Copy link
Contributor Author

mr-russ commented Jul 17, 2024

@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
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants