Skip to content
This repository has been archived by the owner on Sep 10, 2024. It is now read-only.

Relax alphabet restrictions on device IDs #2718

Merged
merged 3 commits into from
May 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 24 additions & 2 deletions crates/data-model/src/compat/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,35 @@ impl Device {
}
}

const fn valid_device_chars(c: char) -> bool {
// This matches the regex in the policy
c.is_ascii_alphanumeric()
|| c == '.'
|| c == '_'
|| c == '~'
|| c == '!'
|| c == '$'
|| c == '&'
|| c == '\''
|| c == '('
|| c == ')'
|| c == '*'
|| c == '+'
|| c == ','
|| c == ';'
|| c == '='
|| c == ':'
|| c == '@'
|| c == '/'
|| c == '-'
}

impl TryFrom<String> for Device {
type Error = InvalidDeviceID;

/// Create a [`Device`] out of an ID, validating the ID has the right shape
fn try_from(id: String) -> Result<Self, Self::Error> {
// This matches the regex in the policy
if !id.chars().all(|c| c.is_ascii_alphanumeric() || c == '-') {
if !id.chars().all(valid_device_chars) {
return Err(InvalidDeviceID::InvalidCharacters);
}

Expand Down
1 change: 1 addition & 0 deletions crates/matrix-synapse/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ serde.workspace = true
tower.workspace = true
tracing.workspace = true
url.workspace = true
urlencoding = "2.1.3"

mas-axum-utils.workspace = true
mas-http.workspace = true
Expand Down
14 changes: 10 additions & 4 deletions crates/matrix-synapse/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ impl HomeserverConnection for SynapseConnection {
err(Display),
)]
async fn query_user(&self, mxid: &str) -> Result<MatrixUser, Self::Error> {
let mxid = urlencoding::encode(mxid);
let mut client = self
.http_client_factory
.client("homeserver.query_user")
Expand Down Expand Up @@ -186,6 +187,7 @@ impl HomeserverConnection for SynapseConnection {
err(Display),
)]
async fn is_localpart_available(&self, localpart: &str) -> Result<bool, Self::Error> {
let localpart = urlencoding::encode(localpart);
let mut client = self
.http_client_factory
.client("homeserver.is_localpart_available");
Expand Down Expand Up @@ -252,11 +254,9 @@ impl HomeserverConnection for SynapseConnection {
.request_bytes_to_body()
.json_request();

let mxid = urlencoding::encode(request.mxid());
let request = self
.put(&format!(
"_synapse/admin/v2/users/{mxid}",
mxid = request.mxid()
))
.put(&format!("_synapse/admin/v2/users/{mxid}"))
.body(body)?;

let response = client.ready().await?.call(request).await?;
Expand All @@ -282,6 +282,7 @@ impl HomeserverConnection for SynapseConnection {
err(Display),
)]
async fn create_device(&self, mxid: &str, device_id: &str) -> Result<(), Self::Error> {
let mxid = urlencoding::encode(mxid);
let mut client = self
.http_client_factory
.client("homeserver.create_device")
Expand Down Expand Up @@ -312,6 +313,8 @@ impl HomeserverConnection for SynapseConnection {
err(Display),
)]
async fn delete_device(&self, mxid: &str, device_id: &str) -> Result<(), Self::Error> {
let mxid = urlencoding::encode(mxid);
let device_id = urlencoding::encode(device_id);
let mut client = self.http_client_factory.client("homeserver.delete_device");

let request = self
Expand Down Expand Up @@ -340,6 +343,7 @@ impl HomeserverConnection for SynapseConnection {
err(Display),
)]
async fn delete_user(&self, mxid: &str, erase: bool) -> Result<(), Self::Error> {
let mxid = urlencoding::encode(mxid);
let mut client = self
.http_client_factory
.client("homeserver.delete_user")
Expand Down Expand Up @@ -370,6 +374,7 @@ impl HomeserverConnection for SynapseConnection {
err(Display),
)]
async fn set_displayname(&self, mxid: &str, displayname: &str) -> Result<(), Self::Error> {
let mxid = urlencoding::encode(mxid);
let mut client = self
.http_client_factory
.client("homeserver.set_displayname")
Expand Down Expand Up @@ -412,6 +417,7 @@ impl HomeserverConnection for SynapseConnection {
err(Display),
)]
async fn allow_cross_signing_reset(&self, mxid: &str) -> Result<(), Self::Error> {
let mxid = urlencoding::encode(mxid);
let mut client = self
.http_client_factory
.client("homeserver.allow_cross_signing_reset")
Expand Down
12 changes: 6 additions & 6 deletions frontend/src/routeTree.gen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { Route as rootRoute } from './routes/__root'
import { Route as ResetCrossSigningImport } from './routes/reset-cross-signing'
import { Route as AccountImport } from './routes/_account'
import { Route as AccountIndexImport } from './routes/_account.index'
import { Route as DevicesIdImport } from './routes/devices.$id'
import { Route as DevicesSplatImport } from './routes/devices.$'
import { Route as ClientsIdImport } from './routes/clients.$id'
import { Route as AccountSessionsIndexImport } from './routes/_account.sessions.index'
import { Route as EmailsIdVerifyImport } from './routes/emails.$id.verify'
Expand All @@ -38,8 +38,8 @@ const AccountIndexRoute = AccountIndexImport.update({
getParentRoute: () => AccountRoute,
} as any)

const DevicesIdRoute = DevicesIdImport.update({
path: '/devices/$id',
const DevicesSplatRoute = DevicesSplatImport.update({
path: '/devices/$',
getParentRoute: () => rootRoute,
} as any)

Expand Down Expand Up @@ -84,8 +84,8 @@ declare module '@tanstack/react-router' {
preLoaderRoute: typeof ClientsIdImport
parentRoute: typeof rootRoute
}
'/devices/$id': {
preLoaderRoute: typeof DevicesIdImport
'/devices/$': {
preLoaderRoute: typeof DevicesSplatImport
parentRoute: typeof rootRoute
}
'/_account/': {
Expand Down Expand Up @@ -122,7 +122,7 @@ export const routeTree = rootRoute.addChildren([
]),
ResetCrossSigningRoute,
ClientsIdRoute,
DevicesIdRoute,
DevicesSplatRoute,
EmailsIdVerifyRoute,
])

Expand Down
8 changes: 4 additions & 4 deletions frontend/src/routes/__root.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,17 @@ export const Route = createRootRouteWithContext<{
case "org.matrix.session_view":
if (search.device_id)
throw redirect({
to: "/devices/$id",
params: { id: search.device_id },
to: "/devices/$",
params: { _splat: search.device_id },
});
throw redirect({ to: "/sessions", search: { last: PAGE_SIZE } });

case "session_end":
case "org.matrix.session_end":
if (search.device_id)
throw redirect({
to: "/devices/$id",
params: { id: search.device_id },
to: "/devices/$",
params: { _splat: search.device_id },
});
throw redirect({ to: "/sessions", search: { last: PAGE_SIZE } });

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ const QUERY = graphql(/* GraphQL */ `
}
`);

export const Route = createFileRoute("/devices/$id")({
export const Route = createFileRoute("/devices/$")({
async loader({ context, params, abortController: { signal } }) {
const viewer = await context.client.query(
CURRENT_VIEWER_QUERY,
Expand All @@ -57,7 +57,7 @@ export const Route = createFileRoute("/devices/$id")({
const result = await context.client.query(
QUERY,
{
deviceId: params.id,
deviceId: params._splat,
userId: viewer.data.viewer.id,
},
{ fetchOptions: { signal } },
Expand All @@ -78,7 +78,7 @@ export const Route = createFileRoute("/devices/$id")({

function NotFound(): React.ReactElement {
const { t } = useTranslation();
const { id: deviceId } = Route.useParams();
const { _splat: deviceId } = Route.useParams();
return (
<Layout>
<Alert
Expand Down
2 changes: 1 addition & 1 deletion policies/authorization_grant.rego
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ allowed_scope("urn:mas:admin") {
allowed_scope(scope) {
# Grant access to the C-S API only if there is a user
interactive_grant_type(input.grant_type)
regex.match("urn:matrix:org.matrix.msc2967.client:device:[A-Za-z0-9-]{10,}", scope)
regex.match("^urn:matrix:org.matrix.msc2967.client:device:[A-Za-z0-9._~!$&'()*+,;=:@/-]{10,}$", scope)
}

allowed_scope("urn:matrix:org.matrix.msc2967.client:api:*") {
Expand Down
16 changes: 0 additions & 16 deletions policies/authorization_grant_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -60,22 +60,6 @@ test_device_scopes {
with input.grant_type as "authorization_code"
with input.scope as "urn:matrix:org.matrix.msc2967.client:device:AAbbCCdd01-asdasdsa1-2313"

# Invalid characters
not allow with input.user as user
with input.client as client
with input.grant_type as "authorization_code"
with input.scope as "urn:matrix:org.matrix.msc2967.client:device:AABB:CCDDEE"

not allow with input.user as user
with input.client as client
with input.grant_type as "authorization_code"
with input.scope as "urn:matrix:org.matrix.msc2967.client:device:AABB*CCDDEE"

not allow with input.user as user
with input.client as client
with input.grant_type as "authorization_code"
with input.scope as "urn:matrix:org.matrix.msc2967.client:device:AABB!CCDDEE"

# Too short
not allow with input.user as user
with input.client as client
Expand Down
Loading