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.

Sign up for GitHub

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

Jump to bottom

MSC1954: Proposal to remove prev_content from the essential keys list #1954

Merged
merged 3 commits into from
Apr 21, 2019
Merged

MSC1954: Proposal to remove prev_content from the essential keys list #1954

merged 3 commits into from
Apr 21, 2019

Conversation

neilisfragile
Copy link
Contributor

@neilisfragile neilisfragile added proposal A matrix spec change proposal proposal-in-review labels Apr 5, 2019
@turt2live turt2live changed the title Proposal to remove prev_content from the essential keys list MSC1954: Proposal to remove prev_content from the essential keys list Apr 5, 2019
@neilisfragile neilisfragile mentioned this pull request Apr 5, 2019
Closed
turt2live
turt2live approved these changes Apr 5, 2019
View reviewed changes
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

seems like the right approach to me

KitsuneRal
KitsuneRal requested changes Apr 6, 2019
View reviewed changes
proposals/1954-remove-prev_event-from-essential-keys-list.md Show resolved Hide resolved
proposals/1954-remove-prev_event-from-essential-keys-list.md Outdated
## Security considerations

I am unaware of any security issues related to this proposal, but can certainly
see issues with a precedent that the federation deviates from the spec.
Copy link
Member

Choose a reason for hiding this comment

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

Actually, keeping prev_content as the current text of the spec says carries a security implication: if somebody stuffed, e.g., illegal or abusive data into a state event and then overwritten the same state event once, those data would be carefully preserved and disseminated via prev_content even after redaction (if implemented as-per-spec). Therefore prev_content is not just inessential, it's harmful if kept by redaction. Not sure if we should add it specifically here but it's probably worth stating at least.

@turt2live
Copy link
Member

in an attempt to help @neilisfragile get this through:

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Apr 9, 2019 •
edited by erikjohnston
Loading

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Apr 9, 2019
@dbkr
Copy link
Member

dbkr commented Apr 9, 2019

Seems entirely reasonable

KitsuneRal
KitsuneRal reviewed Apr 9, 2019
View reviewed changes
proposals/1954-remove-prev_event-from-essential-keys-list.md Outdated
the server(s). Client authors will have to update their code to drop
```prev_content``` - however, given that prev_content should not be used in
important calculations and/or visualisations, this ought to be relatively
uninvaisive change.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uninvaisive change.
uninvasive change.

Copy link
Member

Choose a reason for hiding this comment

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

noninvasive

KitsuneRal
KitsuneRal approved these changes Apr 9, 2019
View reviewed changes
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Approve the contents, modulo the typo

anoadragon453
anoadragon453 reviewed Apr 9, 2019
View reviewed changes
proposals/1954-remove-prev_event-from-essential-keys-list.md Outdated
## Potential issues

It is theoretically possible that a closed federation could exist whose servers
do follow the spec as is. This MSC would render those servers uncompliant with
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
do follow the spec as is. This MSC would render those servers uncompliant with
do follow the spec as is. This MSC would render those servers incompliant with

@anoadragon453
Copy link
Member

Makes sense to me.

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Apr 15, 2019
@mscbot
Copy link
Collaborator

mscbot commented Apr 15, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added finished-final-comment-period and removed final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Apr 21, 2019
@mscbot
Copy link
Collaborator

mscbot commented Apr 21, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@anoadragon453 anoadragon453 merged commit 5792e5f into master Apr 21, 2019
@KitsuneRal KitsuneRal mentioned this pull request Apr 22, 2019
Closed
@turt2live
Copy link
Member

This doesn't need an implementation because the implementation is already correct. The spec is wrong in this scenario. Flagging it as spec-pr-missing and spec-bug due to the nature of the problem.

@turt2live turt2live added spec-bug Something which is in the spec, but is wrong spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed proposal-in-review labels May 20, 2019
@turt2live turt2live self-assigned this May 24, 2019
turt2live added a commit that referenced this pull request May 24, 2019
@turt2live
Remove prev_content from the redaction essential keys list
3c38956 
As per [MSC1954](#1954)

No known changes since the proposal was accepted.
@turt2live turt2live mentioned this pull request May 24, 2019
Merged
@turt2live
Copy link
Member

Spec PR: #2016

@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review merged A proposal whose PR has merged into the spec! and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels May 24, 2019
@turt2live
Copy link
Member

merged 🎉

@aaronraimist aaronraimist mentioned this pull request Jan 12, 2020
Closed
@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anoadragon453 anoadragon453 anoadragon453 left review comments

@turt2live turt2live turt2live approved these changes

@KitsuneRal KitsuneRal KitsuneRal approved these changes

Assignees

@turt2live turt2live

Labels
disposition-merge kind:maintenance MSC which clarifies/updates existing spec merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal spec-bug Something which is in the spec, but is wrong
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@neilisfragile @turt2live @mscbot @dbkr @anoadragon453 @KitsuneRal