Error when Service control urls have question marks

[at1993]

When I tested out the ConsoleTest program, I ran into problems at
device.GetExternalIPAsync() on three different routers.

Open.NAT Information: 0 : Found device at: http://10.10.204.1:1780/InternetGatewayDevice.xml
Open.NAT Information: 0 : 10.10.204.1:1780: Fetching service list
Open.NAT Information: 0 : Found device at: http://10.10.204.1:1780/InternetGatewayDevice.xml
Open.NAT Information: 0 : 10.10.204.1:1780: Parsed services list
Open.NAT Information: 0 : 10.10.204.1:1780: Found service: urn:schemas-upnp-org:service:WANIPConnection:1
Open.NAT Information: 0 : 10.10.204.1:1780: Found upnp service at: /control?WANIPConnection
Open.NAT Information: 0 : 10.10.204.1:1780: Handshake Complete
Open.NAT Information: 0 : Stop Discovery
Open.NAT Verbose: 0 : SOAPACTION: **GetExternalIPAddress** url:http://10.235.204.1:1780/control%3FWANIPConnection
Open.NAT Verbose: 0 : <s:Envelope 
   xmlns:s="http://schemas.xmlsoap.org/soap/envelope/" 
   s:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/">
   <s:Body>
      <u:GetExternalIPAddress xmlns:u="urn:schemas-upnp-org:service:WANIPConnection:1">
      </u:GetExternalIPAddress>
   </s:Body>
</s:Envelope>
Open.NAT Verbose: 0 : WebException status: ServerProtocolViolation

Notice that the UriBuilder replaced the question mark character to %3F. This caused the router to return 404 error.

Solution:

ServiceControlUri = new Uri(string.Format("http://{0}:{1}{2}", locationUri.Host, locationUri.Port, serviceControlUrl));

My second problem is in GetAllMappingsAsync, one of my routers (sagemcom vdsl modem/router)
returns mapping entry with internalclient empty. My fix is as follow

public override async Task<IEnumerable<Mapping>> GetAllMappingsAsync()
{
    var index = 0;
    var mappings = new List<Mapping>();
    while (true)
    {
         try
         {
             var message = new GetGenericPortMappingEntry(index);

             var responseData = await _soapClient
                  .InvokeAsync("GetGenericPortMappingEntry", message.ToXml())
                  .TimeoutAfter(TimeSpan.FromSeconds(4));

             var responseMessage = new GetPortMappingEntryResponseMessage(responseData, DeviceInfo.ServiceType, true);
             if (!string.IsNullOrEmpty(responseMessage.InternalClient))
             {
                  var mapping = new Mapping(responseMessage.Protocol
                      , IPAddress.Parse(responseMessage.InternalClient)
                      , responseMessage.InternalPort
                      , responseMessage.ExternalPort
                      , responseMessage.LeaseDuration
                      , responseMessage.PortMappingDescription);
                  mappings.Add(mapping);
              }
              index++;
        }
        catch (MappingException e)
        {
            System.Diagnostics.Debug.WriteLine(e.ToString());
            if ((e.ErrorCode == UpnpConstants.SpecifiedArrayIndexInvalid) ||
                (e.ErrorCode == UpnpConstants.NoSuchEntryInArray)) break; // there are no more mappings
                    throw;
        }
    }

   return mappings.ToArray();
}

Question: Is there a reason to use UriBuilder if it replaces what it considers illegal characters?

[lontivero]

Thank you very much for reporting these problems. I will fix them asap.

About your question, I used it because it worked so far. Believe me that Open.NAT is hard to test because tehre are a lot of different routers out there. I have to check now if it was a bad idea (and remove it) or if there is something wrong in the way Open.NAT is using it (and fix it).

I will let you know when it is done and also I would request your help for testing the new version.

[at1993]

Thank you Lucas for your prompt reply. I will be more than happy to test your new fix. Cheers!

[at1993]

Hi Lucas, I have another router with DD-WRT. GetAllMappingsAsync failed when the index has exceeded the number of entries in the mapping table. The router returns error 402. So the catch section need to be something like

...
if ((e.ErrorCode == UpnpConstants.SpecifiedArrayIndexInvalid) ||
(e.ErrorCode == UpnpConstants.NoSuchEntryInArray) ||
(e.ErrorCode == UpnpConstants.InvalidArguments)) break; // there are no more mappings

Below is the capture from Wireshark.

[lontivero]

Ummm ok. The weird part of this error is that the router doesn't follow the specs, I think. It should fail with a different error code and description. Anyway, it is something that has to be fixed.

Thank you for your effort. I am happy to know there is a person with a lot of routers using O.N. ;)

[lontivero]

@at1993 ,

• Question mark encoded problem was fixed.
• DD-WRT linux based router returns 402-Invalid Argument. It was verified and the DD-WRT code is water clear (you can see it below). This means your fix is the only solution.

map = upnp_portmap_with_index(context, ARG_UI2(in_NewPortMappingIndex));
if (!map)
     return SOAP_INVALID_ARGS; 

• Sagemcom modem/router. I have problems with this one because the spec is clear: NewInternalClient CANNOT be wildcard (ie. empty). What you propose is check if responseMessage.InternalClient is empty and in that case just skip the entry. Evidently, if the entry is invalid your fix is ok but, could you send the soap request and response to me? I need to understand the problem better.

Thank you

[at1993]

The Sagemcom has a Games/Applications section which allows user a quick way of to do port forwarding. I believe those empty entries port mapping entries are simply a place holders of this list. I will verify that when I get some free time. Attached is the request and response.

[lontivero]

What you say makes sense for me. Empty NewInternalClient, NewInternalPort=0, NewEnabled=0. It seems to be a place holders or templace in the list for the Microsoft Media Server. Moreover, the code fails if NewInternalClient is empty so, I will use your code.

[at1993]

Hi Lucas, I have a Linksys WRT1900AC. It is not answering to SSDP (M-Search) sent during discovery. UPnp is enabled at the router. Do you have any idea?

[at1993]

False alarm :) It was the anti-virus thing. Open.NAT is OK for WRT1900AC.

I do have a proposal though. I would like to use Open.NAT in one of my application which managed the "Internet of Things". Some of these gadgets needs to be accessible from outside the router. The app will manage the port mappings when necessary. In Open.NAT, the PrivateIP is forced to DeviceInfo.LocalAddress in CreatePortMapAsync. Two things need to be modified as followed.

1. Change one of the Mapping constructor to public.

public Mapping(Protocol protocol, IPAddress privateIP, int privatePort, int publicPort)
: this(protocol, privateIP, privatePort, publicPort, 0, "Open.Nat")
{
}

2.Check PrivateIP before before setting it to DeviceInfo.LocalAddress

public override async Task CreatePortMapAsync(Mapping mapping)
{
Guard.IsNotNull(mapping, "mapping");
if (mapping.PrivateIP==null) mapping.PrivateIP = DeviceInfo.LocalAddress;
...
}

[lontivero]

I think that is not a problem given it does not introduce any breaking change. Let me fix the defects first.

[lontivero]

Done. Defects were fixed and are available for real testing in the BugFixing branch. Could you confirme everything is working correctly now, @at1993 please?

[at1993]

Hi Lucas, the BugFixing branch works. Thank you very much!

[lontivero]

Ok, I will merge it back to master and generate a new nuget package with these fixes and the new feature for allowing the creation of mappings with arbitrary privateIP.

I estimate it can be done on Wednesday. In the meanwhile you can work with your own fork or just wait a couple of days.

[at1993]

NP. and thank you.

[at1993]

When calling GetSpecificMappingAsync under DD-WRT it returns errorcode 402. Please see soap request & response attached.

[lontivero]

I should have realized that the same problem would happen in GetSpecificMappingAsync method too. I will fix it in the BugFixing branch today.

Thanks for reporting, @at1993

[lontivero]

Fixed on 790b407