Bug 1640575 - Move "debug view tagging" to Rust (#998)

* Add debug view tag management capabilites to rust

* Expose debug view tag management through ffi

* Remove ping tag management from Android in favor of rust impl

* Remove ping tag management from iOS in favor of rust impl

glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt

@@ -585,6 +585,21 @@ open class GleanInternalAPI internal constructor () {
         }
     }
 
+    /**
+     * Set a tag to be applied to headers when uploading pings for debug view.
+     * This is only meant to be used internally by the `GleanDebugActivity`.
+     *
+     * @param value The value of the tag, which must be a valid HTTP header value.
+     */
+    fun setDebugViewTag(value: String): Boolean {
+        if (isInitialized()) {
+            return LibGleanFFI.INSTANCE.glean_set_debug_view_tag(value).toBoolean()
+        } else {
+            Log.e(LOG_TAG, "Glean must be initialized before setting a debug view tag.")
+            return false
+        }
+    }
+
     /**
      * TEST ONLY FUNCTION.
      * This is called by the GleanTestRule, to enable test mode.

glean-core/android/src/main/java/mozilla/telemetry/glean/config/Configuration.kt

@@ -55,8 +55,6 @@ internal class FfiConfiguration(
  * @property logPings whether to log ping contents to the console. This is only meant to be used
  *           internally by the `GleanDebugActivity`.
  * @property httpClient The HTTP client implementation to use for uploading pings.
- * @property pingTag String tag to be applied to headers when uploading pings for debug view.
- *           This is only meant to be used internally by the `GleanDebugActivity`.
  * @property channel the release channel the application is on, if known. This will be
  *           sent along with all the pings, in the `client_info` section.
  */
@@ -68,8 +66,7 @@ data class Configuration internal constructor(
     // NOTE: since only simple object or strings can be made `const val`s, if the
     // default values for the lines below are ever changed, they are required
     // to change in the public constructor below.
-    val httpClient: PingUploader = HttpURLConnectionUploader(),
-    val pingTag: String? = null
+    val httpClient: PingUploader = HttpURLConnectionUploader()
 ) {
     /**
      * Configuration for Glean.
@@ -97,7 +94,6 @@ data class Configuration internal constructor(
         maxEvents = maxEvents,
         logPings = DEFAULT_LOG_PINGS,
         httpClient = httpClient,
-        pingTag = null,
         channel = channel
     )
 

glean-core/android/src/main/java/mozilla/telemetry/glean/debug/GleanDebugActivity.kt

@@ -37,9 +37,6 @@ class GleanDebugActivity : Activity() {
          * The value must match the pattern `[a-zA-Z0-9-]{1,20}`.
          */
         const val TAG_DEBUG_VIEW_EXTRA_KEY = "tagPings"
-
-        // Regular expression filter for debugId
-        internal val pingTagPattern = "[a-zA-Z0-9-]{1,20}".toRegex()
     }
 
     // IMPORTANT: These activities are unsecured, and may be triggered by
@@ -83,19 +80,15 @@ class GleanDebugActivity : Activity() {
 
             // Check for ping debug view tag to apply to the X-Debug-ID header when uploading the
             // ping to the endpoint
-            var pingTag: String? = intent.getStringExtra(TAG_DEBUG_VIEW_EXTRA_KEY)
+            var debugViewTag: String? = intent.getStringExtra(TAG_DEBUG_VIEW_EXTRA_KEY)
 
-            // Validate the ping tag against the regex pattern
-            pingTag?.let {
-                if (!pingTagPattern.matches(it)) {
-                    Log.e(LOG_TAG, "tagPings value $it does not match accepted pattern $pingTagPattern")
-                    pingTag = null
-                }
+            // Set the debug view tag, if the tag is invalid it won't be set
+            debugViewTag?.let {
+                Glean.setDebugViewTag(debugViewTag)
             }
 
             val debugConfig = Glean.configuration.copy(
-                logPings = intent.getBooleanExtra(LOG_PINGS_EXTRA_KEY, Glean.configuration.logPings),
-                pingTag = pingTag
+                logPings = intent.getBooleanExtra(LOG_PINGS_EXTRA_KEY, Glean.configuration.logPings)
             )
 
             // Finally set the default configuration before starting

glean-core/android/src/main/java/mozilla/telemetry/glean/net/Upload.kt

@@ -10,7 +10,6 @@ import org.json.JSONException
 import com.sun.jna.Structure
 import com.sun.jna.Pointer
 import com.sun.jna.Union
-import mozilla.telemetry.glean.Glean
 import mozilla.telemetry.glean.rust.getRustString
 import mozilla.telemetry.glean.rust.RustBuffer
 
@@ -93,10 +92,6 @@ internal class PingRequest(
             Log.e(LOG_TAG, "Error while parsing headers for ping $documentId")
         }
 
-        Glean.configuration.pingTag?.let {
-            headers.add(Pair("X-Debug-ID", it))
-        }
-
         return headers
     }
 

glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt

@@ -560,6 +560,8 @@ internal interface LibGleanFFI : Library {
 
     fun glean_process_ping_upload_response(task: FfiPingUploadTask.ByReference, status: Int)
 
+    fun glean_set_debug_view_tag(value: String): Byte
+
     // Misc
 
     fun glean_str_free(ptr: Pointer)

glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt

@@ -104,14 +104,14 @@ class GleanTest {
     }
 
     @Test
-    fun `X-Debug-ID header is correctly added when pingTag is not null`() {
+    fun `X-Debug-ID header is correctly added when debug view tag is set`() {
         val server = getMockWebServer()
         resetGlean(context, Glean.configuration.copy(
             serverEndpoint = "http://" + server.hostName + ":" + server.port,
-            logPings = true,
-            pingTag = "this-ping-is-tagged"
+            logPings = true
         ))
 
+        Glean.setDebugViewTag("this-ping-is-tagged")
         Glean.handleBackgroundEvent()
 
         // Now trigger it to upload

glean-core/ffi/glean.h

@@ -642,6 +642,8 @@ uint8_t glean_quantity_test_has_value(uint64_t metric_id, FfiStr storage_name);
 
 void glean_register_ping_type(uint64_t ping_type_handle);
 
+uint8_t glean_set_debug_view_tag(FfiStr tag);
+
 void glean_set_dirty_flag(uint8_t flag);
 
 void glean_set_experiment_active(FfiStr experiment_id,

glean-core/ffi/src/lib.rs

@@ -492,4 +492,12 @@ pub extern "C" fn glean_initialize_standalone_uploader(data_dir: FfiStr) -> u8 {
     })
 }
 
+#[no_mangle]
+pub extern "C" fn glean_set_debug_view_tag(tag: FfiStr) -> u8 {
+    with_glean_mut(|glean| {
+        let tag = tag.to_string_fallible()?;
+        Ok(glean.set_debug_view_tag(&tag))
+    })
+}
+
 define_string_destructor!(glean_str_free);

glean-core/ios/Glean/Config/Configuration.swift

@@ -8,7 +8,6 @@ public struct Configuration {
     let serverEndpoint: String
     var logPings: Bool
     let maxEvents: Int32?
-    var pingTag: String?
     let channel: String?
 
     struct Constants {
@@ -24,21 +23,17 @@ public struct Configuration {
     ///   * logPings: whether to log ping contents to the console.
     ///     This is only meant to be used internally by the `GleanDebugActivity`.
     ///   * maxEvents: the number of events to store before the events ping is sent.
-    ///   * pingTag: String tag to be applied to headers when uploading pings for debug view.
-    ///     Used internally by the `GleanDebugActivity`.
     ///   * channel: the release channel the application is on, if known.
     ///     This will be sent along with all the pings, in the `client_info` section.
     internal init(
         serverEndpoint: String = Constants.defaultTelemetryEndpoint,
         logPings: Bool = Constants.defaultLogPings,
         maxEvents: Int32? = nil,
-        pingTag: String? = nil,
         channel: String? = nil
     ) {
         self.serverEndpoint = serverEndpoint
         self.logPings = logPings
         self.maxEvents = maxEvents
-        self.pingTag = pingTag
         self.channel = channel
     }
 
@@ -56,7 +51,6 @@ public struct Configuration {
         self.serverEndpoint = serverEndpoint ?? Constants.defaultTelemetryEndpoint
         self.logPings = Constants.defaultLogPings
         self.maxEvents = maxEvents
-        self.pingTag = nil
         self.channel = channel
     }
 

glean-core/ios/Glean/Debug/GleanDebugTools.swift

@@ -9,10 +9,6 @@ class GleanDebugUtility {
     // This struct is used for organizational purposes to keep the class constants in a single place
     struct Constants {
         static let logTag = "glean/GleanDebugUtility"
-        // Since ping tags are transmitted via HTTP in a header, and since they are displayed to the
-        // user on the Glean Debug View, we need to constrain the tag to a safe character set and
-        // limit the length to prevent possible attack vectors.
-        static let pingTagRegexPattern = "^[a-zA-Z0-9-]{1,20}$"
     }
 
     private static let logger = Logger(tag: Constants.logTag)
@@ -58,8 +54,12 @@ class GleanDebugUtility {
 
         if let parsedCommands = processCustomUrlQuery(urlQueryItems: params) {
             if let tag = parsedCommands.pingTag {
-                Glean.shared.configuration?.pingTag = tag
-                logger.debug("Pings tagged with: \(tag)")
+                if Glean.shared.setDebugViewTag(tag) {
+                    logger.debug("Pings tagged with: \(tag)")
+                } else {
+                    logger.error("Invalid ping tag name, aborting Glean debug tools")
+                    return
+                }
             }
 
             if let logPings = parsedCommands.logPings {
@@ -107,11 +107,6 @@ class GleanDebugUtility {
                     return nil
                 }
 
-                if !param.value!.matches(Constants.pingTagRegexPattern) {
-                    logger.error("Invalid ping tag name, aborting Glean debug tools")
-                    return nil
-                }
-
                 pingTagName = param.value
             case "logPings":
                 if willLogPings != nil {

glean-core/ios/Glean/Glean.swift

@@ -435,6 +435,20 @@ public class Glean {
         }
     }
 
+    /// Set a tag to be applied to headers when uploading pings for debug view.
+    /// This is only meant to be used internally by the `GleanDebugActivity`.
+    ///
+    /// - parameters:
+    ///     * value: The value of the tag, which must be a valid HTTP header value.
+    public func setDebugViewTag(_ value: String) -> Bool {
+        if self.isInitialized() {
+            return glean_set_debug_view_tag(value).toBool()
+        } else {
+            self.logger.error("Glean must be initialized before setting a debug view tag.")
+            return false
+        }
+    }
+
     /// When applications are launched using the custom URL scheme, this helper function will process
     /// the URL and parse the debug commands
     ///

glean-core/ios/Glean/GleanFfi.h

@@ -642,6 +642,8 @@ uint8_t glean_quantity_test_has_value(uint64_t metric_id, FfiStr storage_name);
 
 void glean_register_ping_type(uint64_t ping_type_handle);
 
+uint8_t glean_set_debug_view_tag(FfiStr tag);
+
 void glean_set_dirty_flag(uint8_t flag);
 
 void glean_set_experiment_active(FfiStr experiment_id,

glean-core/ios/Glean/Net/HttpPingUploader.swift

@@ -87,10 +87,6 @@ public class HttpPingUploader {
             // See https://github.com/AliSoftware/OHHTTPStubs#known-limitations.
             request.httpBody = data
 
-            if let tag = config.pingTag {
-                request.addValue(tag, forHTTPHeaderField: "X-Debug-ID")
-            }
-
             return request
         }
 

glean-core/ios/Glean/Net/Upload.swift

@@ -59,12 +59,6 @@ struct PingRequest {
         self.documentId = documentId
         self.path = path
         self.body = body
-
-        var headers = headers
-        if let pingTag = Glean.shared.configuration?.pingTag {
-            headers["X-Debug-ID"] = pingTag
-        }
-
         self.headers = headers
     }
 }

glean-core/ios/GleanTests/Config/ConfigurationTests.swift

@@ -31,10 +31,6 @@ class ConfigurationTests: XCTestCase {
             config?.maxEvents,
             "Default max events are set"
         )
-        XCTAssertNil(
-            config?.pingTag,
-            "Default pingTag is set"
-        )
         XCTAssertNil(
             config?.channel,
             "Default channel is set"

glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift

@@ -19,25 +19,6 @@ class GleanDebugUtilityTests: XCTestCase {
         OHHTTPStubs.removeAllStubs()
     }
 
-    func testHandleCustomUrlTagPings() {
-        // Check invalid tags: This should have the configuration keep the
-        // default value of nil because they don't match the accepted
-        // regex for ping tag names.
-        var url = URL(string: "test://glean?tagPings=This-tag-is-not-valid-because-it-is-too-long")
-        Glean.shared.handleCustomUrl(url: url!)
-        XCTAssertNil(Glean.shared.configuration?.pingTag)
-
-        url = URL(string: "test://glean?tagPings=Invalid_Chars@!!$")
-        Glean.shared.handleCustomUrl(url: url!)
-        XCTAssertNil(Glean.shared.configuration?.pingTag)
-
-        // Check valid tag: This should update the pingTag value of the
-        // configuration object.
-        url = URL(string: "test://glean?tagPings=Glean-Ping")
-        Glean.shared.handleCustomUrl(url: url!)
-        XCTAssertEqual("Glean-Ping", Glean.shared.configuration?.pingTag)
-    }
-
     func testHandleCustomUrlLogPings() {
         // Test initial state
         XCTAssertFalse(Glean.shared.configuration!.logPings)

glean-core/ios/GleanTests/Net/HttpPingUploaderTests.swift

@@ -12,8 +12,7 @@ class HttpPingUploaderTests: XCTestCase {
     private let testPath = "/some/random/path/not/important"
     private let testPing = "{ \"ping\": \"test\" }"
     private let testConfig = Configuration(
-        logPings: true,
-        pingTag: "Tag"
+        logPings: true
     )
 
     override func tearDown() {