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

Allow reports with long comments from remote instances, but truncate #25028

Conversation

ThisIsMissEm
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm commented May 17, 2023

This is an attempted fix for #24975. I didn't understand exactly the logic for the following, which is to say my confidence in this PR not having unintended side-effects is low, despite all the tests passing:

  def local?
    false # Force uri_for to use uri attribute
  end

So added explicit url_for and uri_for handling for Flag's (reports), making local? become delegated to if the account making the report is local or not.

I do note that in url_for, the line return target.url if target.respond_to?(:local?) && !target.local? may not work if the target only has a uri and not url property/method, I'm not sure if this is intentional or not?

Further, I noticed that generate_uri_for is returning not the value the code implies it may be returning, and that there was no test coverage for using ReportService to create local reports, despite this being how ReportService is used (it's used for both API reports and ActivityPub Flag's)

@renchap renchap added the bug Something isn't working label May 19, 2023
@ClearlyClaire ClearlyClaire self-requested a review May 22, 2023 10:59
Comment on lines +43 to +46
# A report is considered local if the reporter is local
delegate :local?, to: :account

validates :comment, length: { maximum: 1_000 }, if: :local?
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, you could rewrite the test as something like if: -> { account&.local? }

@ClearlyClaire
Copy link
Contributor

I didn't understand exactly the logic for the following, which is to say my confidence in this PR not having unintended side-effects is low, despite all the tests passing:

  def local?
    false # Force uri_for to use uri attribute
  end

So added explicit url_for and uri_for handling for Flag's (reports), making local? become delegated to if the account making the report is local or not.

I'll double-check the possible consequences, but that sounds about right.

@ClearlyClaire
Copy link
Contributor

I do note that in url_for, the line return target.url if target.respond_to?(:local?) && !target.local? may not work if the target only has a uri and not url property/method, I'm not sure if this is intentional or not?

I would say this is intentional, however, I am not actually sure all code making use of url_for expect it to be possibly nil, this might be worth double-checking.

@ClearlyClaire ClearlyClaire merged commit 19f9098 into mastodon:main May 22, 2023
23 checks passed
@ClearlyClaire ClearlyClaire added the to backport PR needed to be backported label May 22, 2023
@ThisIsMissEm ThisIsMissEm deleted the allow-remote-reports-with-long-comments branch July 3, 2023 11:35
nrdufour added a commit to nrdufour/home-ops that referenced this pull request Sep 7, 2023
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [ghcr.io/mastodon/mastodon](https://github.com/mastodon/mastodon) | patch | `v4.1.6` -> `v4.1.7` |

---

### Release Notes

<details>
<summary>mastodon/mastodon (ghcr.io/mastodon/mastodon)</summary>

### [`v4.1.7`](https://github.com/mastodon/mastodon/releases/tag/v4.1.7)

[Compare Source](mastodon/mastodon@v4.1.6...v4.1.7)

<h1><picture>
  <source media="(prefers-color-scheme: dark)" srcset="./lib/assets/wordmark.dark.png?raw=true">
  <source media="(prefers-color-scheme: light)" srcset="./lib/assets/wordmark.light.png?raw=true">
  <img alt="Mastodon" src="./lib/assets/wordmark.light.png?raw=true" height="34">
</picture></h1>

#### Changelog
##### Changed

-   Change remote report processing to accept reports with long comments, but truncate them ([ThisIsMissEm](mastodon/mastodon#25028))

##### Fixed

-   **Fix blocking subdomains of an already-blocked domain** ([ClearlyClaire](mastodon/mastodon#26392))
-   Fix `/api/v1/timelines/tag/:hashtag` allowing for unauthenticated access when public preview is disabled ([danielmbrasil](mastodon/mastodon#26237))
-   Fix inefficiencies in `PlainTextFormatter` ([ClearlyClaire](mastodon/mastodon#26727))

#### Upgrade notes

To get the code for v4.1.7, use `git fetch && git checkout v4.1.7`.

> As always, **make sure you have backups of the database before performing any upgrades**. If you are using docker-compose, this is how a backup command might look: `docker exec mastodon_db_1 pg_dump -Fc -U postgres postgres > name_of_the_backup.dump`

##### Dependencies

External dependencies have not changed compared to v4.1.6, the compatible Ruby, PostgreSQL, Node, Elasticsearch and Redis versions are the same, that is:

-   Ruby: 2.7 to 3.0
-   PostgreSQL: 9.5 or newer
-   Elasticsearch (optional, for full-text search): 7.x
-   Redis: 4 or newer
-   Node: >= 14, < 18
-   ImageMagick: 6.9.7-7 or newer

> If your uploaded images are broken after the upgrade, it means your installed ImageMagick version is older than the new minimum version (6.9.7-7), for example if you are running Ubuntu 18.04. If this happens, you can find more information and ways to fix it [on this page](mastodon/mastodon#25776).

##### Update steps

The following instructions are for updating from 4.1.6.

If you are upgrading directly from an earlier release, please carefully read the upgrade notes for the skipped releases as well, as they often require extra steps such as database migrations.

**Non-Docker only:**

1.  Install dependencies: `bundle install` and `yarn install --frozen-lockfile`

**Both Docker and non-Docker:**

1.  Restart all Mastodon processes

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4yMy4yIiwidXBkYXRlZEluVmVyIjoiMzYuMjMuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Reviewed-on: https://git.home/nrdufour/home-ops/pulls/74
Co-authored-by: Renovate <renovate@ptinem.io>
Co-committed-by: Renovate <renovate@ptinem.io>
@ClearlyClaire ClearlyClaire removed the to backport PR needed to be backported label Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants