Correctly parse domains from userIds (#1068)

* Only split userId on the first colon

* Add changelog

* Update changelog to also account for IPv6

* Fix split in permissions

* Update changelog to reflect permissions fix as well

* update changelog

* lint

---------

Co-authored-by: Twilight Sparkle <[email protected]>

Author: Half-Shot

changelog.d/1068.bugfix

@@ -0,0 +1 @@
+Fix bugs in handling server names that includes colons.

src/CommentProcessor.ts

@@ -1,4 +1,4 @@
-import { Appservice } from "matrix-bot-sdk";
+import { Appservice, UserID } from "matrix-bot-sdk";
 import markdown from "markdown-it";
 import emoji from "node-emoji";
 import { MatrixMessageContent, MatrixEvent } from "./MatrixEvent";
@@ -142,7 +142,8 @@ export class CommentProcessor {
     for (const [full, userId] of mentionMatches) {
       if (this.as.isNamespacedUser(userId)) {
         // XXX: Prefix hack
-        const githubId = userId.split(":")[0].substr("@_github_".length);
+        const parsedUserID = new UserID(userId);
+        const githubId = parsedUserID.localpart.slice("@_github_".length);
         if (!githubId) {
           continue;
         }

src/Connections/GenericHook.ts

@@ -9,7 +9,7 @@ import { Logger } from "matrix-appservice-bridge";
 import { MessageSenderClient } from "../MatrixSender";
 import markdownit from "markdown-it";
 import { MatrixEvent } from "../MatrixEvent";
-import { Appservice, Intent, StateEvent } from "matrix-bot-sdk";
+import { Appservice, Intent, StateEvent, UserID } from "matrix-bot-sdk";
 import { ApiError, ErrCode } from "../api";
 import { BaseConnection } from "./BaseConnection";
 import { BridgeConfigGenericWebhooks } from "../config/sections";
@@ -455,7 +455,7 @@ export class GenericHookConnection
     if (!this.config.userIdPrefix) {
       return this.intent.userId;
     }
-    const [, domain] = this.intent.userId.split(":");
+    const { domain } = new UserID(this.intent.userId);
     const name =
       this.state.name &&
       this.state.name

src/IntentUtils.ts

@@ -1,5 +1,5 @@
 import { Logger } from "matrix-appservice-bridge";
-import { Appservice, Intent, MatrixClient } from "matrix-bot-sdk";
+import { Appservice, Intent, MatrixClient, UserID } from "matrix-bot-sdk";
 import axios from "axios";
 
 const log = new Logger("IntentUtils");
@@ -44,7 +44,7 @@ export async function getIntentForUser(
   as: Appservice,
   prefix?: string,
 ) {
-  const domain = as.botUserId.split(":")[1];
+  const { domain } = new UserID(as.botUserId);
   const intent = as.getIntentForUserId(
     `@${prefix ?? ""}${user.login}:${domain}`,
   );

src/config/permissions.rs

@@ -100,12 +100,10 @@ impl BridgePermissions {
         service: String,
         permission: String,
     ) -> napi::Result<bool> {
-        let parts: Vec<&str> = mxid.split(':').collect();
         let permission_int = permission_level_to_int(permission)?;
-        let domain = if parts.len() > 1 {
-            parts[1].to_string()
-        } else {
-            parts[0].to_string()
+        let domain: String = match mxid.split_once(':') {
+            Some((.., d)) => d.to_string(),
+            None => return Ok(false),
         };
         for actor_permission in self.config.iter() {
             // Room_id
@@ -128,12 +126,10 @@ impl BridgePermissions {
 
     #[napi]
     pub fn check_action_any(&self, mxid: String, permission: String) -> napi::Result<bool> {
-        let parts: Vec<&str> = mxid.split(':').collect();
         let permission_int = permission_level_to_int(permission)?;
-        let domain = if parts.len() > 1 {
-            parts[1].to_string()
-        } else {
-            parts[0].to_string()
+        let domain: String = match mxid.split_once(':') {
+            Some((.., d)) => d.to_string(),
+            None => return Ok(false),
         };
         for actor_permission in self.config.iter() {
             if !self.match_actor(actor_permission, &domain, &mxid) {

tests/config/permissions.ts

@@ -80,6 +80,53 @@ describe("Config/BridgePermissions", () => {
         .to.be.true;
     });
 
+    it("handles domain actors with ports", () => {
+      const bridgePermissionNoPort = genBridgePermissions(
+        "bar",
+        "my-service",
+        "login",
+      );
+      expect(
+        bridgePermissionNoPort.checkAction(
+          "@foo:bar:9999",
+          "my-service",
+          "login",
+        ),
+      ).to.be.false;
+      expect(
+        bridgePermissionNoPort.checkAction("@foo:bar", "my-service", "login"),
+      ).to.be.true;
+      const bridgePermissionWithPort = genBridgePermissions(
+        "bar:9999",
+        "my-service",
+        "login",
+      );
+      expect(
+        bridgePermissionWithPort.checkAction(
+          "@foo:bar:9999",
+          "my-service",
+          "login",
+        ),
+      ).to.be.true;
+      expect(
+        bridgePermissionWithPort.checkAction(
+          "@foo:bar:999",
+          "my-service",
+          "login",
+        ),
+      ).to.be.false;
+      expect(
+        bridgePermissionWithPort.checkAction(
+          "@foo:bar:",
+          "my-service",
+          "login",
+        ),
+      ).to.be.false;
+      expect(
+        bridgePermissionWithPort.checkAction("@foo:bar", "my-service", "login"),
+      ).to.be.false;
+    });
+
     it("will return true for a wildcard actor", () => {
       const bridgePermissions = genBridgePermissions(
         "*",