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

Fix punycode encoding of the local domain #21440

Merged
merged 1 commit into from
Dec 15, 2022
Merged

Conversation

Tritlo
Copy link
Contributor

@Tritlo Tritlo commented Nov 23, 2022

This adds a call to punycode.toUnicode to prettify the local domain, changing e.g. @matti@xn--lofll-1sat.is to @matti@loðfíll.is.
This happens on the:

  • local user profile page
  • the footer
  • the about page
  • ..., everywhere {domain} is used directly.

@Tritlo Tritlo changed the title Fix punycode encoding on account headers [WIP] Fix punycode encoding of the local domain Nov 23, 2022
@ineffyble ineffyble linked an issue Nov 23, 2022 that may be closed by this pull request
@Tritlo Tritlo changed the title [WIP] Fix punycode encoding of the local domain Fix punycode encoding of the local domain Nov 23, 2022
@Tritlo
Copy link
Contributor Author

Tritlo commented Nov 23, 2022

Sorry for all the force pushes... I missed a few places in my first attempt. Should be good to go, please review! If you want to see it in action, it's working great over at https://loðfíll.is.

@gol-cha
Copy link
Contributor

gol-cha commented Nov 23, 2022

I think it would be better to add prettified local domain to initial-state. Sorry if this is misplaced.
e.g. Add the following line to InitialStateSerializer
pretty_domain: Addressable::IDNA.to_unicode(instance_presenter.domain)

@Tritlo
Copy link
Contributor Author

Tritlo commented Nov 23, 2022

My intuition is that we should be doing it during rendering, since it is just a pretty rendering of a particular string when the context says it's a URL. But I might be wrong here.

@ClearlyClaire
Copy link
Contributor

I think it would indeed be much simpler to just have the domain from the initial_state contain the unicode representation.

@Tritlo
Copy link
Contributor Author

Tritlo commented Nov 23, 2022

I'm not sure that would be correct, sometimes you want the punycoded domain, where the unicode representation would not work. That's the whole idea behind punycode!

Similarly, there are some instances of remoteDomain and localDomain as well, where using punycode is appropriate as well. I think it's safer to apply the "rendering" of the punycode precisely when it's being rendered, but otherwise maintain the URLs as-is, since links and such require the original (non-rendered) domain.

@ClearlyClaire
Copy link
Contributor

I think domain here is pretty much used only for display purposes, though. But I may be wrong, I'd have to double-check.

@ineffyble ineffyble added the ui Front-end, design label Nov 24, 2022
@ineffyble
Copy link
Member

Is any defence against homonym attacks needed here? Do we run the risk of fake Mastodon instance impersonating real ones using indistinguishable unicode charts?

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

The remote domains derived from account's acct attributes are already utf8-encoded rather than punycode-encoded because the acct attribute is “prettified” server-side, so no change is needed client-side there.

I think the local domain passed down from the initial state is only ever used for display, so I think it should be passed down as utf8-encoded rather than prettified in various places on the client-side.

One place I am not entirely sure what the best course of action would be is app/javascript/mastodon/components/domain.js, as the server-side code currently returns punycode-encoded domains from the API. So either we indeed need to take care of that client-side, or change the API for consistency with acct.

app/javascript/mastodon/components/status_action_bar.js Outdated Show resolved Hide resolved
app/javascript/mastodon/containers/status_container.js Outdated Show resolved Hide resolved
@ClearlyClaire
Copy link
Contributor

Is any defence against homonym attacks needed here? Do we run the risk of fake Mastodon instance impersonating real ones using indistinguishable unicode charts?

I'd say we do not need defense against homonym attacks for the local domain, since the local domain would be controlled by the attacker anyway. We may need defense against homonym attacks for remote domains, but this is not what the current PR is about.

@ineffyble
Copy link
Member

We may need defense against homonym attacks for remote domains, but this is not what the current PR is about.

Ah, right, I saw the references to encoding the remote domains in the code, but if they're already encoded then nothing to worry about for this PR.

@Tritlo
Copy link
Contributor Author

Tritlo commented Nov 25, 2022

I removed the unneccesary ones, per @ClearlyClaire. I still think this is the way to go, to render the domain when it's rendered. But I can change it so that the domain is prettified before being sent, if you think it's safe.

@ClearlyClaire
Copy link
Contributor

I removed the unneccesary ones, per @ClearlyClaire. I still think this is the way to go, to render the domain when it's rendered. But I can change it so that the domain is prettified before being sent, if you think it's safe.

I do think it is safe, yes.

@Tritlo
Copy link
Contributor Author

Tritlo commented Nov 25, 2022

Alright, much simpler now!

(sorry for the all the pushes, my git got into some weird state there for a second).

@ClearlyClaire
Copy link
Contributor

Alright, much simpler now!

Thanks again for the PR!

(sorry for the all the pushes, my git got into some weird state there for a second).

No worries!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui Front-end, design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Punycode-Domain is displayed raw since update to version 4.0.2
6 participants