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

Add confirmation when redirecting logged-out requests to permalink #27792

Merged
merged 9 commits into from Jan 24, 2024

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Nov 9, 2023

If you open a link in the form of https://mastodon.social/@alice@example.com while logged out, Mastodon will immediately redirect you to https://example.com/@alice. While this is good for many reasons, it also creates potential for phishing, as the user clicking the mastodon.social link may be convinced they are opening a mastodon.social page. We will now show a screen in that scenario that tells you explicitly that you are leaving mastodon.social.

This does not affect logged in browsing, or even normal browsing while logged out, as the redirect only occurs when you open such a page directly, not when navigating to it from the already loaded web interface.

Screen_Shot_2023-10-30_at_03 50 38

@Gargron Gargron added the ui Front-end, design label Nov 9, 2023
@ClearlyClaire
Copy link
Contributor

I think this is a good change overall, though I'm slightly worried about the friction this may add.

Hitting F5 while logged out on some remote content now presents this page instead of silently redirecting, which is expected. But trying to navigate back stays on the same page (I assume the browser is trying to do history API-based navigation because that's how that particular history state was reached, but this page does not have anything to handle these history API events). This is a confusing regression, but I am not sure how it should be fixed.

@ClearlyClaire
Copy link
Contributor

ClearlyClaire commented Nov 13, 2023

Another thing I'm noticing is that it breaks the redirect with Accept: application/activity+json queries by showing the confirmation unconditionally.

Hitting F5 while logged out on some remote content now presents this page instead of silently redirecting, which is expected. But trying to navigate back stays on the same page (I assume the browser is trying to do history API-based navigation because that's how that particular history state was reached, but this page does not have anything to handle these history API events). This is a confusing regression, but I am not sure how it should be fixed.

Redirecting to an intermediary path (e.g. /redirect/accounts/:id itself rendering the confirmation) instead of rendering in the application path fixes this.

Pushed commits addressing those issues.

@ClearlyClaire
Copy link
Contributor

A few notes: at @Gargron's request, I merged the two redirect endpoints into one, using a signed query parameter, but I do not like that solution very much because:

  • it results in long (potentially too long) query strings
  • it uses crypto when not really needed
  • it prevents us from knowing anything about the account/post before showing the link, e.g. honoring suspensions

In addition, there is an issue with link previews, in that with the automatic redirection, link previews used to work even when users shared “non-canonical” links to a remote resource, but that won't work with the confirmation page. If we revert to passing post/account ID, we can generate our own OpenGraph tags for that, which I guess would be better, if still not ideal.

@Joshix-1
Copy link

we can generate our own OpenGraph tags for that, which I guess would be better

That wouldn't be good. The instance of the original post should be the one to generate OpenGraph. Then they can decide whether they want to allow that. Generating OpenGraph for content from other instances is imho not a good idea.

Maybe the open redirect could stay for certain bot user agents

@ThisIsMissEm
Copy link
Contributor

Might be good to do a comparison of what other link roadblocks like there look like. I know both twitter, linkedin and instagram have these, but I couldn't find any UX analysis on them

@ClearlyClaire
Copy link
Contributor

ClearlyClaire commented Jan 17, 2024

we can generate our own OpenGraph tags for that, which I guess would be better

That wouldn't be good. The instance of the original post should be the one to generate OpenGraph. Then they can decide whether they want to allow that. Generating OpenGraph for content from other instances is imho not a good idea.

Yeah, you're right, that makes sense. Maybe giving up on OpenGraph in that case and “punishing” users for using the “wrong” URL is the best course of action. Still not really satisfactory given how easy it is to do.

Maybe the open redirect could stay for certain bot user agents

I think this would preclude reverse-proxy caching.

@Joshix-1
Copy link

Maybe giving up on OpenGraph in that case and “punishing” users for using the “wrong” URL is the best course of action.

I'd really dislike that. I always thought the current system is really clever.

@ClearlyClaire
Copy link
Contributor

I rebased and force-pushed because a yanked dependency made CI fail. In the meantime, the controller specs had been rewritten as request specs and the rebase uncovered new tests failures. I fixed them.

Copy link

codecov bot commented Jan 22, 2024

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (7ecf7f5) 85.08% compared to head (1ece1ed) 85.08%.
Report is 2 commits behind head on main.

Files Patch % Lines
app/lib/permalink_redirector.rb 66.66% 6 Missing and 3 partials ⚠️
app/controllers/redirect/base_controller.rb 66.66% 4 Missing ⚠️
app/controllers/redirect/accounts_controller.rb 60.00% 2 Missing ⚠️
app/controllers/redirect/statuses_controller.rb 60.00% 2 Missing ⚠️
...controllers/concerns/web_app_controller_concern.rb 85.71% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #27792    +/-   ##
========================================
  Coverage   85.08%   85.08%            
========================================
  Files        1038     1041     +3     
  Lines       28147    28294   +147     
  Branches     4532     4566    +34     
========================================
+ Hits        23948    24074   +126     
- Misses       3039     3060    +21     
  Partials     1160     1160            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Sponsor Member

@renchap renchap left a comment

Choose a reason for hiding this comment

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

Looks good to me.

After internal discussions, we chose to go with this solution for now, and not worry about Opengraph data. We will check how it behaves in real world, and if the lack of Opengraph data is really a widespread issue, we will try to implement it.

So far, the 3 alternatives to implement it would be:

  • generate it locally. This is not ideal as it might not be the same as the remote data, or the remote server might not want to provide OG for this object
  • download and cache the OG data from the remote URL, and serve it. Not ideal as we need to make a synchronous request, and have a list of what meta tags we want to use
  • use a double-redirect method to serve an HTTP confirmation page for human users, and a 302 otherwise (to bot users, for example those making OG requests). This would involve a first 302 redirect to an internal endpoint, setting a cookie. Then the second endpoint can check if the cookie is there to see if this is a real user or not. This might not work in real life, and machine users might set the cookie on the redirect, so this would need to be tested further.

@Gargron Gargron added this pull request to the merge queue Jan 22, 2024
@renchap renchap removed this pull request from the merge queue due to a manual request Jan 22, 2024
@Joshix-1
Copy link

Why would you just remove a feature? That could destroy Opengraph for the whole fediverse for a fairly long time.

if the lack of Opengraph data is really a widespread issue

How will you meausure that? Did you measure first if it currently gets used? (You could do that with your big instances)

@renchap
Copy link
Sponsor Member

renchap commented Jan 22, 2024

Why would you just remove a feature?

We have not found a way to have it done easily. I outlined the 3 ways we see to do it above, but they are not without problems and/or add significant complexity.

Maybe we missed an easy way to do it?

That could destroy Opengraph for the whole fediverse for a fairly long time.

Only when people are using non-canonical URLs for sharing

if the lack of Opengraph data is really a widespread issue

How will you meausure that? Did you measure first if it currently gets used? (You could do that with your big instances)

As we do not collect data, we mostly rely on user feedback for this.

@Gargron
Copy link
Member Author

Gargron commented Jan 22, 2024

Why would you just remove a feature? That could destroy Opengraph for the whole fediverse for a fairly long time.

For the record, we're not removing anything. We never generated OpenGraph previews for remote content, and none of this affects OpenGraph previews for local content. If people use the "Copy link" option on posts and profiles, there will be no issues when sharing that link on other platforms.

@Joshix-1
Copy link

But I see people often copy the url from the browser bar and send that. And thanks to the redirect Open Graph is generated (if the original poster (or the instance) wants that)
That's a great feature

@Gargron Gargron added this pull request to the merge queue Jan 24, 2024
Merged via the queue into main with commit b19ae52 Jan 24, 2024
57 checks passed
@Gargron Gargron deleted the feature-redirect-screen branch January 24, 2024 10:54
neatchee pushed a commit to neatchee/mastodon that referenced this pull request Feb 4, 2024
* Add confirmation when redirecting logged-out requests to permalink (mastodon#27792)

Co-authored-by: Claire <claire.github-309c@sitedethib.com>

* chore(deps): update dependency rubocop to v1.60.1 (mastodon#28731)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Fix `Rails/WhereExists` cop in app/lib (mastodon#28862)

* Use active variants for boost icons and increase icon size (mastodon#27924)

* chore(deps): update dependency haml_lint to v0.55.0 (mastodon#28856)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update dependency selenium-webdriver to v4.17.0 (mastodon#28858)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update artifact actions (major) to v4 (major) (mastodon#28415)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency dotenv to v16.4.0 (mastodon#28872)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update dependency chewy to v7.5.0 (mastodon#28730)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Tidy up association declaration in `Instance` model (mastodon#28880)

* Fix process of receiving posts with bearcaps is not working (mastodon#26527)

* Fix redirect confirmation for accounts (mastodon#28902)

* Add tests for redirect confirmations (mastodon#28903)

* Add tests for processing statuses using bearcap URIs (mastodon#28904)

* chore(deps): update dependency rspec-rails to v6.1.1 (mastodon#28905)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency axios to v1.6.6 (mastodon#28895)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Fix remaining `Rails/WhereExists` cop violations, regenerate todo (mastodon#28892)

* fix(deps): update dependency dotenv to v16.4.1 (mastodon#28889)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Add `CustomFilterKeyword#to_regex` method (mastodon#28893)

* New Crowdin Translations (automated) (mastodon#28899)

Co-authored-by: GitHub Actions <noreply@github.com>

* Consolidate db test prep steps to rake task (mastodon#28886)

* Fix `Style/TernaryParentheses` cop (mastodon#28387)

* Reduce factory creation in `spec/models/account_statuses_cleanup_policy` (mastodon#28361)

* Extract helper methods for db connection and table existence check in `CLI::Maintenance` task (mastodon#28281)

* Refactor conversations components in web UI (mastodon#28833)

Co-authored-by: Claire <claire.github-309c@sitedethib.com>

* Fix `Style/GuardClause` cop in app/controllers (mastodon#28420)

* Update paperclip and climate_control gems (mastodon#28379)

* Extract `rebuild_index` method in maintenance CLI (mastodon#28911)

* Add specs for `Instance` model scopes and add `with_domain_follows` scope (mastodon#28767)

* [Glitch] Add confirmation when redirecting logged-out requests to permalink

Port SCSS changes from b19ae52 to glitch-soc

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Use active variants for boost icons and increase icon size

Port 5a838ce to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* Fix CSS loading in redirect controller

* [Glitch] Refactor conversations components in web UI (glitch-soc#2589)

Port 3205a65 to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>
Co-authored-by: Eugen Rochko <eugen@zeonfederated.com>

* Fix crash in private mention conversations in glitch-soc flavor (glitch-soc#2595)

* Remove obsolete locale file (glitch-soc#2596)

* Add github action workflow for manual security builds (mastodon#29040)

* Adapt workflow to glitch-soc

* Fix missing `workflow_dispatch` trigger for `build-security` (mastodon#29041)

* Configure selenium to use Chrome version 120 (mastodon#29038)

* Fix security builds not being marked latest

* Merge pull request from GHSA-3fjr-858r-92rw

* Fix insufficient origin validation

* Bump version to 4.3.0-alpha.1

* Fix build-security workflow for glitch-soc

* Restore -streaming suffix for security builds (glitch-soc#2602)

* Temporary hack to correctly tag the security docker image…

* Fix build-security docker tags

* fix lint issues in conversation.jsx

---------

Signed-off-by: Claire <claire.github-309c@sitedethib.com>
Co-authored-by: Eugen Rochko <eugen@zeonfederated.com>
Co-authored-by: Claire <claire.github-309c@sitedethib.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Matt Jankowski <matt@jankowski.online>
Co-authored-by: KMY(雪あすか) <tt@kmycode.net>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: GitHub Actions <noreply@github.com>
Co-authored-by: Aaron Brady <aaron@insom.me.uk>
neatchee added a commit to neatchee/mastodon that referenced this pull request Feb 4, 2024
* Add confirmation when redirecting logged-out requests to permalink (mastodon#27792)

Co-authored-by: Claire <claire.github-309c@sitedethib.com>

* chore(deps): update dependency rubocop to v1.60.1 (mastodon#28731)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Fix `Rails/WhereExists` cop in app/lib (mastodon#28862)

* Use active variants for boost icons and increase icon size (mastodon#27924)

* chore(deps): update dependency haml_lint to v0.55.0 (mastodon#28856)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update dependency selenium-webdriver to v4.17.0 (mastodon#28858)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update artifact actions (major) to v4 (major) (mastodon#28415)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency dotenv to v16.4.0 (mastodon#28872)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update dependency chewy to v7.5.0 (mastodon#28730)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Tidy up association declaration in `Instance` model (mastodon#28880)

* Fix process of receiving posts with bearcaps is not working (mastodon#26527)

* Fix redirect confirmation for accounts (mastodon#28902)

* Add tests for redirect confirmations (mastodon#28903)

* Add tests for processing statuses using bearcap URIs (mastodon#28904)

* chore(deps): update dependency rspec-rails to v6.1.1 (mastodon#28905)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency axios to v1.6.6 (mastodon#28895)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Fix remaining `Rails/WhereExists` cop violations, regenerate todo (mastodon#28892)

* fix(deps): update dependency dotenv to v16.4.1 (mastodon#28889)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Add `CustomFilterKeyword#to_regex` method (mastodon#28893)

* New Crowdin Translations (automated) (mastodon#28899)

Co-authored-by: GitHub Actions <noreply@github.com>

* Consolidate db test prep steps to rake task (mastodon#28886)

* Fix `Style/TernaryParentheses` cop (mastodon#28387)

* Reduce factory creation in `spec/models/account_statuses_cleanup_policy` (mastodon#28361)

* Extract helper methods for db connection and table existence check in `CLI::Maintenance` task (mastodon#28281)

* Refactor conversations components in web UI (mastodon#28833)

Co-authored-by: Claire <claire.github-309c@sitedethib.com>

* Fix `Style/GuardClause` cop in app/controllers (mastodon#28420)

* Update paperclip and climate_control gems (mastodon#28379)

* Extract `rebuild_index` method in maintenance CLI (mastodon#28911)

* Add specs for `Instance` model scopes and add `with_domain_follows` scope (mastodon#28767)

* [Glitch] Add confirmation when redirecting logged-out requests to permalink

Port SCSS changes from b19ae52 to glitch-soc

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Use active variants for boost icons and increase icon size

Port 5a838ce to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* Fix CSS loading in redirect controller

* [Glitch] Refactor conversations components in web UI (glitch-soc#2589)

Port 3205a65 to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>
Co-authored-by: Eugen Rochko <eugen@zeonfederated.com>

* Fix crash in private mention conversations in glitch-soc flavor (glitch-soc#2595)

* Remove obsolete locale file (glitch-soc#2596)

* Add github action workflow for manual security builds (mastodon#29040)

* Adapt workflow to glitch-soc

* Fix missing `workflow_dispatch` trigger for `build-security` (mastodon#29041)

* Configure selenium to use Chrome version 120 (mastodon#29038)

* Fix security builds not being marked latest

* Merge pull request from GHSA-3fjr-858r-92rw

* Fix insufficient origin validation

* Bump version to 4.3.0-alpha.1

* Fix build-security workflow for glitch-soc

* Restore -streaming suffix for security builds (glitch-soc#2602)

* Temporary hack to correctly tag the security docker image…

* Fix build-security docker tags

---------

Signed-off-by: Claire <claire.github-309c@sitedethib.com>
Co-authored-by: Eugen Rochko <eugen@zeonfederated.com>
Co-authored-by: Claire <claire.github-309c@sitedethib.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Matt Jankowski <matt@jankowski.online>
Co-authored-by: KMY(雪あすか) <tt@kmycode.net>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: GitHub Actions <noreply@github.com>
Co-authored-by: Aaron Brady <aaron@insom.me.uk>
@lapcat
Copy link

lapcat commented Feb 9, 2024

My web browser extension Homecoming for Mastodon relies on the redirect, so I find this change to be very unfortunate.

It actually has the feel of when Twitter censored Mastodon links.

https://underpassapp.com/homecoming/

@lapcat
Copy link

lapcat commented Feb 9, 2024

The distributed nature of Mastodon is already difficult for users. This change makes it worse in my opinion. We should be finding ways of making navigation between instances easier not harder.

@lapcat
Copy link

lapcat commented Feb 9, 2024

As we do not collect data, we mostly rely on user feedback for this.

Was this change very motivated by any user feedback about an alleged phishing issue?

I don't know why we're now teaching users to be distrustful of other Mastodon instances. The redirect page sows distrust, as if visiting other instances is harmful. That seems counterproductive, a self-inflicted wound.

@ClearlyClaire
Copy link
Contributor

My web browser extension Homecoming for Mastodon relies on the redirect, so I find this change to be very unfortunate.

Is there something we can do about that, e.g. a meta tag on the redirect screen making it extra clear what is happening?

I don't know why we're now teaching users to be distrustful of other Mastodon instances. The redirect page sows distrust, as if visiting other instances is harmful. That seems counterproductive, a self-inflicted wound.

Maybe the wording can be changed, but the reason for the confirmation screen is basically that you do not end up on a different instance than yours thinking it is yours. e.g. you were logged in, but for some reason your authentication lapsed, you fail to notice you are redirected somewhere else, and proceed to give your credentials. Or someone crafts a link that purposefully redirects you to something that looks like your instance's log-in screen.

@lapcat
Copy link

lapcat commented Feb 12, 2024

Is there something we can do about that, e.g. a meta tag on the redirect screen making it extra clear what is happening?

I don't want to get into the business of parsing responses as HTML, but there are a couple of things that would help:

First, if the User-Agent of the request is curl, then restore the old behavior rather than giving the /redirect/ URL. That would fix my extension immediately. (I'm not actually using curl, but I spoof the User-Agent as curl.) The redirect HTML page is for web browsers, not for tools such as curl.

Second, provide a specific HTTP API to get the redirected URL, analogous to the /authorize_interaction?uri= API for example.

Or someone crafts a link that purposefully redirects you to something that looks like your instance's log-in screen.

This seems like a rather obscure and unlikely attack. Are there many malicious Mastodon instances? (I mean malware, not simply hosting abhorrent posts.) I'm assuming that one Mastodon instance won't redirect to another Mastodon instance if the latter is defederated, correct? If my assumption is incorrect, then that might be most of the problem.

Anyway, you'd need a maliciously-crafted Mastodon instance, post a mastodon.myinstance/@foobar link somewhere, somehow find a logged-out user of mastodon.myinstance, hope they're not suspicious of the @foobar path of the URL, induce them to click the URL, and trick them into entering their credentials. Has this ever happened?

@lapcat
Copy link

lapcat commented Feb 12, 2024

I think the bigger issue here, beyond just my extension, is that 99.99% of the time, when someone posts a mastodon.myinstance link, they're simply copying/sharing it from their own web browser for non-malicious purposes, and this new redirect screen makes Mastodon a lot more unfriendly, especially to people who don't yet use Mastodon and are immediately taught to fear Mastodon links.

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.

None yet

6 participants