IOS/KD: Add WiiLink as WC24 service provider by default

[noahpistilli]

As discussed, this PR has WiiLink as the default service for WiiConnect24 services by default (More are coming!)

Currently, there is a checkbox users can toggle in the config to disable or enable WiiLink. If enabled, it will reroute requests by a supportted channel to the WiiLink equivalent.

Image of the config option under Wii

TODO:
Somewhere to put the Terms of Service visibly. JMC had the idea of having it popup much like the Analytics disclaimer.

[leoetlino]

The changes look sane - I wonder if we should use a dropdown instead of a checkbox though, just in case we decide to add more WC24 service provider in the future. Also, I think it'd be nice to have a description of what WiiLink is in the tooltip text.

[leoetlino]

Alternatively, let's add an INI override for the server names to e.g. make local testing/development of WiiLink style services easier

[noahpistilli]

Added an INI field as requested, as well as rename Enable WiiLink to Enable WiiConnect24. If Enable WiiConnect24 is checked, Dolphin will load the default INI for the WiiConnect24 title. Otherwise, it will load the user's INI.

I have also added WiiLink URLs to the default INI. Nintendo Channel is also currently not in a complete state as main requests use SSL.

[leoetlino]

Looks like MSVC is unhappy about operator< being used on bools; this is a side effect of the wrong overload of TryParse getting selected.

Consider adding a specialisation of TryParse for boolean enums: https://godbolt.org/z/xPn5dzzof

// add this:
template <typename T> requires (detail::IsBooleanEnum<T>())
bool TryParse(T* output) {
    bool value;
    if (!TryParse(&value))
        return false;
    
    *output = static_cast<T>(value);
    return true;
}

// and then change the integer/enum template to this:
template <typename T>
requires (std::is_integral_v<T> || (std::is_enum_v<T> && !detail::IsBooleanEnum<T>()))
bool TryParse(T* output);

[noahpistilli]

Just added the patches in the INI files for all regions of the Nintendo Channel. Should be good to merge now.

[AdmiralCurtiss]

Can you please add end-of-file newlines to all your inis?

[AdmiralCurtiss]

This include should go into the block with the other Core includes. Maybe also add a newline between the Core block and the VideoCommon one while you're there.

[AdmiralCurtiss]

This function doesn't seem to be used anywhere?

[AdmiralCurtiss]

This seems too broad, this would also replace instances outside the domain part.

[AdmiralCurtiss]

This also seems too aggressive. You'd be replacing any instances outside the Host header, too.

[AdmiralCurtiss]

This on the other hand is still clearly wrong. You're targetting the Host: part of the HTTP header here, yeah? https://developer.mozilla.org/en-US/docs/Web/HTTP/Messages In that case, you should also only search in that area of the request. Basically, something like (untested if this actually works, you might want to write a unit test for the logic because it seems prone to off-by-one errors...)

  size_t pos = 0;
  while (pos < source.size())
  {
    if (source[pos] == '\n')  // end of header
      break;

    if (source.substr(pos).starts_with("Host:"))
    {
      size_t end_of_line = source.find('\n', pos);
      std::string_view domain =
          source.substr(pos + 5, end_of_line == std::string_view::npos ? std::string_view::npos :
                                                                         (end_of_line - pos - 5));
      for (const NetworkPatch& patch : s_patches)
      {
        if (domain.trim() == patch.source)
        {
          return source.substr(0, pos) + "Host: " + patch.replacement +
                 (end_of_line == std::string_view::npos ? "" : source.substr(end_of_line));
        }
      }
    }
    else
    {
      size_t end_of_line = source.find('\n', pos);
      if (end_of_line == std::string_view::npos)
        break;
      pos = end_of_line + 1;
    }
  }
  return std::nullopt;

[noahpistilli]

This example was a big help. With some testing and looking at RFC2616, all HTTP messages have CRLF line endings, so I opted for checking for that instead of just \n. Other than that and the return line, it works perfectly with my testing in the Nintendo Channel.

[AdmiralCurtiss]

Aside from those this seems fine. I'll grant that it's not particularly likely that those domains would appear anywhere outside the specific places you're expecting them, but it still doesn't seem a great idea to run a blanket search-and-replace when you could be more targetted to what you're actually expecting.

[AdmiralCurtiss]

This is better but I feel you could still trick this if you really tried to...

It's probably fine given typical input but if you feel motivated you might want to actually extract the host part of the URI based on https://www.rfc-editor.org/rfc/rfc3986 and just target that part.

[noahpistilli]

If you really want me to I can fix it, otherwise I might just defer it to another time.

[AdmiralCurtiss]

Eh... like I said, if you feel motivated. You'd have to actively try to abuse this, and to questionable effect.

[AdmiralCurtiss]

This should probably default to off.

[AdmiralCurtiss]

The size_t end_of_line = source.find("\r\n", pos); can be pulled out of the branch.

There's a mismatch here between the starts_with("Host:") and .substr(pos + 6, you're dropping a byte here. I'm not sure whether HTTP forces a space between the : and the start of the data? Might be okay to just change to starts_with("Host: ")?

Adding a few consts to variables here wouldn't hurt either.

[AdmiralCurtiss]

This logic here also feels a bit jank now that I think about it... I would expect user patches to apply in addition to the system ones, regardless of if the WiiLink option is enabled or not. Though I'd be okay with that being a follow-up by either you or someone else, I'm not expecting every user to suddenly start writing URL replacements into their INIs.

[noahpistilli]

The logic I was trying to get at here is that the WiiLink URL's will be contained within the INI's we pack. If Enable WiiLink is off, it would allow for users to put in their own replacement URL if they please. I just didn't want any conflicts between the INI's URL's

[AdmiralCurtiss]

Untested, but this looks reasonable enough to me now.

[noahpistilli]

Upon reviewing payload sizes and how they look, all Nintendo Channel payloads were data_size - 2. This is because the literal FR was at the end of the string, not sure what it is for but it should not be sent through the socket.

[AdmiralCurtiss]

Oh, duh, yeah that's not a nullterminated string so we're treating it incorrectly. That explains your corruption. Please change it to

          const std::optional<std::string> patch = WC24PatchEngine::GetNetworkPatchByPayload(
              std::string_view(data, BufferInSize));
          if (patch)
          {
            data = patch->c_str();
            BufferInSize = static_cast<u32>(patch->size());
          }

and see if it's correct then.

[noahpistilli]

Works like a charm.