Fix connectivity tests ios 873

[buggmagnet]

This PR fixes 3 tests that were constantly failing on the CI runner

• testAPIConnectionViaBridges
• testAPIReachableWhenBlocked
• testAppStillFunctioningWhenAPIDown

How was it fixed ?

testAPIConnectionViaBridges Is skipped at the moment, the only shadowsocks bridge provided by the staging environment is declared as inactive, hence the app cannot use shadowsocks bridges to connect to the API.

testAPIReachableWhenBlocked This was a bug, we never updated the TransportMonitor to let API calls go through the packet tunnel when entering the blocked state.

testAppStillFunctioningWhenAPIDown was failing because an accessibility element was put on the wrong item in API access methods.

On top of that, since we added encrypted DNS proxy methods, we never added an encrypted DNS resolver for staging environments. This is now fixed.

---

This change is 

[linear]

IOS-873 Fix connectivity tests

[buggmagnet]

Reviewable status: 0 of 15 files reviewed, 1 unresolved discussion

---

mullvad-encrypted-dns-proxy/src/config_resolver.rs line 65 at r1 (raw file):

pub async fn resolve_default_config(domain: &str) -> Result<Vec<config::ProxyConfig>, Error> {
    // TODO: We should remove the default value here and just force the callers to provide a domain instead

Note to self: Remove this TODO

[MarkusPettersson98]

Reviewed 4 of 15 files at r1, all commit messages.
Reviewable status: 4 of 15 files reviewed, 4 unresolved discussions (waiting on @buggmagnet)

---

mullvad-encrypted-dns-proxy/src/config_resolver.rs line 64 at r1 (raw file):

}

pub async fn resolve_default_config(domain: &str) -> Result<Vec<config::ProxyConfig>, Error> {

It would be appreciated with a comment on what this function actually does. I know it wasn't there before, but imo it is even more important now when the function accepts arguments 🙏

Code quote:

pub async fn resolve_default_config(domain: &str) -> Result<Vec<config::ProxyConfig>, Error> {

---

mullvad-encrypted-dns-proxy/src/state.rs line 62 at r1 (raw file):

    /// Fetch a config, but error out only when no existing configuration was there.
    pub async fn fetch_configs(&mut self, domain: &str) -> Result<(), FetchConfigError> {

⛏️

/// Fetch a config from [domain], but error out only when no existing configuration was there.
pub async fn fetch_configs(&mut self, domain: &str) -> Result<(), Fetc

Code quote:

    /// Fetch a config, but error out only when no existing configuration was there.
    pub async fn fetch_configs(&mut self, domain: &str) -> Result<(), FetchConfigError> {

---

mullvad-ios/src/encrypted_dns_proxy.rs line 92 at r1 (raw file):

        let c_str = CStr::from_ptr(domain_name);
        String::from_utf8_lossy(c_str.to_bytes())
    };

The scope of the unsafe block could be narrowed here. Consider changing this to

let domain = {
    let c_str = unsafe { CStr::from_ptr(domain_name) };
    String::from_utf8_lossy(c_str.to_bytes())
};

And while doing so, it would be good practice to provide a SAFETY comment when using unsafe. Consider adding a comment on what guarantees the caller needs to uphold in order for encrypted_dns_proxy_init to be safe!

/// Initializes a valid pointer to an instance of `EncryptedDnsProxyState`.
///
/// # Safety
///
/// * [domain_name] must not be non-null.
///
/// * [domain_name] pointer must be [valid](core::ptr#safety)
///
/// * The caller must ensure that the pointer to the [domain_name] string contains a nul terminator
/// at the end of the string.
#[no_mangle]
pub unsafe extern "C" fn encrypted_dns_proxy_init(
    domain_name: *const c_char,
) -> *mut EncryptedDnsProxyState {
    let domain = {
        // SAFETY: domain_name points to a valid region of memory and contains a nul terminator.
        let c_str = unsafe { CStr::from_ptr(domain_name) };
        String::from_utf8_lossy(c_str.to_bytes())
    };
  ..
}

Suggestion:

    let domain = {
        let c_str = unsafe { CStr::from_ptr(domain_name) };
        String::from_utf8_lossy(c_str.to_bytes())
    };

[buggmagnet]

Reviewable status: 4 of 15 files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)

---

mullvad-encrypted-dns-proxy/src/config_resolver.rs line 64 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

It would be appreciated with a comment on what this function actually does. I know it wasn't there before, but imo it is even more important now when the function accepts arguments 🙏

Done.

---

mullvad-encrypted-dns-proxy/src/state.rs line 62 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

⛏️

/// Fetch a config from [domain], but error out only when no existing configuration was there.
pub async fn fetch_configs(&mut self, domain: &str) -> Result<(), Fetc

Done.

---

mullvad-ios/src/encrypted_dns_proxy.rs line 92 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

The scope of the unsafe block could be narrowed here. Consider changing this to

let domain = {
    let c_str = unsafe { CStr::from_ptr(domain_name) };
    String::from_utf8_lossy(c_str.to_bytes())
};

And while doing so, it would be good practice to provide a SAFETY comment when using unsafe. Consider adding a comment on what guarantees the caller needs to uphold in order for encrypted_dns_proxy_init to be safe!

/// Initializes a valid pointer to an instance of `EncryptedDnsProxyState`.
///
/// # Safety
///
/// * [domain_name] must not be non-null.
///
/// * [domain_name] pointer must be [valid](core::ptr#safety)
///
/// * The caller must ensure that the pointer to the [domain_name] string contains a nul terminator
/// at the end of the string.
#[no_mangle]
pub unsafe extern "C" fn encrypted_dns_proxy_init(
    domain_name: *const c_char,
) -> *mut EncryptedDnsProxyState {
    let domain = {
        // SAFETY: domain_name points to a valid region of memory and contains a nul terminator.
        let c_str = unsafe { CStr::from_ptr(domain_name) };
        String::from_utf8_lossy(c_str.to_bytes())
    };
  ..
}

Done.

[MarkusPettersson98]

Reviewed 3 of 4 files at r2, all commit messages.
Reviewable status: 4 of 15 files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

---

mullvad-ios/src/encrypted_dns_proxy.rs line 92 at r1 (raw file):

Previously, buggmagnet wrote…

Done.

Please add a SAFETY comment next to the unsafe block. See my original comment for some inspiration 😉

[buggmagnet]

Reviewable status: 1 of 15 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

---

mullvad-ios/src/encrypted_dns_proxy.rs line 92 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Please add a SAFETY comment next to the unsafe block. See my original comment for some inspiration 😉

Sorry, I misread your suggestion. It should be fixed now.

[MarkusPettersson98]

Reviewed 2 of 4 files at r4, 1 of 2 files at r5, all commit messages.
Reviewable status: 4 of 15 files reviewed, all discussions resolved

[MarkusPettersson98]

Reviewed all commit messages.
Reviewable status: 4 of 15 files reviewed, all discussions resolved

---

mullvad-ios/src/encrypted_dns_proxy.rs line 100 at r6 (raw file):

    // SAFETY: domain_name points to a valid region of memory and contains a nul terminator.
    let domain = {
        let c_str = unsafe { CStr::from_ptr(domain_name) };

⛏️ The SAFETY comment could be adjacent to the unsafe block, i.e. move it to the next line 👼

Code quote:

    // SAFETY: domain_name points to a valid region of memory and contains a nul terminator.
    let domain = {
        let c_str = unsafe { CStr::from_ptr(domain_name) };

[buggmagnet]

Reviewable status: 3 of 15 files reviewed, all discussions resolved (waiting on @MarkusPettersson98)

---

mullvad-ios/src/encrypted_dns_proxy.rs line 100 at r6 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

⛏️ The SAFETY comment could be adjacent to the unsafe block, i.e. move it to the next line 👼

Done ! 🫡

[MarkusPettersson98]

Reviewed 1 of 1 files at r7.
Reviewable status: 4 of 15 files reviewed, all discussions resolved

---

mullvad-ios/src/encrypted_dns_proxy.rs line 100 at r6 (raw file):

Previously, buggmagnet wrote…

Done ! 🫡

🙌

[acb-mv]

Reviewed 8 of 15 files at r1, 2 of 2 files at r6, all commit messages.
Reviewable status: 14 of 15 files reviewed, all discussions resolved

---

ios/MullvadVPN/TransportMonitor/TransportMonitor.swift line 47 at r7 (raw file):

    }

    private func shouldByPassVPN(tunnel: any TunnelProtocol) -> Bool {

This isn't related to the work in this PR, but why is "ByPass" bicapitalised (as opposed to "Bypass")?

[buggmagnet]

Reviewable status: 14 of 15 files reviewed, all discussions resolved

---

ios/MullvadVPN/TransportMonitor/TransportMonitor.swift line 47 at r7 (raw file):

Previously, acb-mv wrote…

This isn't related to the work in this PR, but why is "ByPass" bicapitalised (as opposed to "Bypass")?

The person who wrote this wasn't a native english speaker. I'll issue a 🥷 fix for it, nice catch !

[mojganii]

Reviewable status: 14 of 15 files reviewed, all discussions resolved

[github-actions]

🚨 End to end tests failed. Please check the failed workflow run.