Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client side validation of alias max length #4060

Merged
merged 6 commits into from
Oct 4, 2021

Conversation

BillCarsonFr
Copy link
Member

Fixes #3934

Adding client side limitation on room alias length

image

Just a maxlength on input with a counter

@@ -94,6 +94,8 @@ class SpaceDetailEpoxyController @Inject constructor(
hint(host.stringProvider.getString(R.string.create_space_alias_hint))
suffixText(":" + data.homeServerName)
prefixText("#")
// spaces alias are limited to 255
maxLength(255)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would you feel about a shared constant across all the usages?

something like const val MAX_SPACE_ALIAS_LENGTH = 255 could potentially allow us to replace the comment as well as the variable name would have the same information

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, thanks, in a general rule, we avoid magic numbers.
Also, I do not see such limitation in the doc (https://matrix.org/docs/spec/client_server/r0.6.1#room-aliases).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah the length limitation is defined here: https://matrix.org/docs/spec/appendices#room-aliases
Heads up that the limitation counts the sign # and the domain so a computation is needed here.
@BillCarsonFr if you add this link above the const you will defined, I would be super happy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm taking care of it

@github-actions
Copy link

github-actions bot commented Sep 22, 2021

Unit Test Results

  42 files  ±0    42 suites  ±0   48s ⏱️ ±0s
  83 tests ±0    83 ✔️ ±0  0 💤 ±0  0 ±0 
220 runs  ±0  220 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 4af9e2c. ± Comparison against base commit 4af9e2c.

♻️ This comment has been updated with latest results.

@BillCarsonFr BillCarsonFr added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Sep 23, 2021
@@ -165,6 +165,8 @@ object MatrixPatterns {
fun candidateAliasFromRoomName(name: String): String {
return Regex("\\s").replace(name.lowercase(), "_").let {
"[^a-z0-9._%#@=+-]".toRegex().replace(it, "")
}.let { alias ->
if (alias.length > 255) alias.substring(0, 255) else alias
Copy link
Member

@bmarty bmarty Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would simplify by always taking the substring here, no need to check the length.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also deduce the domain length + 1 (for #)

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see our remarks

@bmarty bmarty force-pushed the feature/bca/space_validate_alias branch from f9afa4a to 7636b4d Compare October 4, 2021 10:41
/**
* Max length for an alias. Room aliases MUST NOT exceed 255 bytes (including the # sigil and the domain).
* See [maxAliasLocalPartLength]
* Ref. https://matrix.org/docs/spec/appendices#room-aliases
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@bmarty
Copy link
Member

bmarty commented Oct 4, 2021

Updated:

image

fun candidateAliasFromRoomName(roomName: String, domain: String): String {
return roomName.lowercase()
// Replace spaces by '_'
.let { Regex("\\s").replace(it, "_") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: these comments could be replaced by extensions with the same name as the comment (and potentially be reused)

roomName
  .lowercase()
  .replaceSpacesWithUnderscore()
  .removeInvalidRoomNameChars()
  .limitLength(length = MatrixConstants.maxAliasLocalPartLength(domain))


fun String.replaceSpacesWithUnderscore() = Regex("\\s").replace(this, "_")
fun String.removeInvalidRoomNameChars() = "[^a-z0-9._%#@=+-]".toRegex().replace(this, "")
fun String.limitLength(length: Int) = this.substring(0, length)

I'm more in the self documenting code camp than adding comments to describe what the code does, where possible!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 done in f385e74

@bmarty bmarty merged commit 4af9e2c into develop Oct 4, 2021
@bmarty bmarty deleted the feature/bca/space_validate_alias branch October 4, 2021 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate public space address length when user clicks away from
3 participants