microsoft / wopi-validator-core

A tool to test and validate server-side WOPI implementations
MIT License
42 stars 34 forks source link

PathOverride should be inserted before '?' if it exists #78

Closed Stranger-Ok closed 5 years ago

Stranger-Ok commented 5 years ago

We have additional logic at backend checking licence by additional parameters in URL. This definitely working Ok with Office Server before. But to implement Office online integration we should validate our's endpoint with wopi-core-validator. And it failing on some tests. Actual problem: Url parsing in methods BuildWopiUri and GetEndpointAddressOverride not always working correctly. If Url will already contain ? with parameters Validator will add /contents in the end of URI string and will break it. Example: Executing command: Microsoft.Office.Validator.exe ... -w http://localhost:5000/wopi/files/1111?customerId=123 Will create a request to next url: http://localhost:5000/wopi/files/1111?customerId=123/contents Solution: Check in both methods if URL already contains ? than insert PathOverride before ? else add to the end.

Stranger-Ok commented 5 years ago

Fixing first place:

abstract class WopiRequest : RequestBase
{
    ...

    protected Uri BuildWopiUri(string url, string accessToken, long accessTokenTtl)
    {
     var indexOfQuestionmark = url.IndexOf("?");

     StringBuilder sb = new StringBuilder(url);
     // insert PathOverride before "?" if it exists, otherwise in the end
     sb.Insert(indexOfQuestionmark >= 0 ? indexOfQuestionmark : url.Length, PathOverride);

     // add any required connecting stuff to make it valid to add
     // a query string param key=value pair
     if (indexOfQuestionmark < 0)
            ...
    }
    ...
}

And second:

abstract class RequestBase : IRequest
{
    ...
    protected string GetEndpointAddressOverride(Dictionary<string, string> savedState)
    {
     ...
     else
     {
    var indexOfQuestionmark = urlToUse.IndexOf("?");

    UriBuilder builder = new UriBuilder(urlToUse);
    // insert PathOverride before "?" if it exists, otherwise in the end
    builder.Path.Insert(indexOfQuestionmark >= 0 ? indexOfQuestionmark : urlToUse.Length, PathOverride);

    return builder.ToString();
    }
    }
    ...
}
tylerbutler commented 5 years ago

This is the correct behavior, per the WOPI spec. WOPI defines the file ID as "everything after /wopi/files/ in the WopiSrc URL". Thus, given a WopiSrc value like this: http://localhost:5000/wopi/files/1111?customerId=123, a WOPI server would interpret the file ID as 1111?customerId=123. Thus, adding /contents to the WopiSrc is the correct way to create a file contents endpoint URL from a WopiSrc. Nothing in the WOPI spec obligates a client or server to interpret the file ID as anything other than a string.

Stranger-Ok commented 5 years ago

Thanks a lot! This means that we can't use own parameters in URL, I didn't find any restrictions about this in documentation. That's why I created this issue.