Page MenuHomePhabricator

Topics that are deleted then suppressed expose topic title in public deletion log
Closed, ResolvedPublic

Description

If a topic is deleted, then suppressed, the topic title is exposed in the public deletion log.

Spun off of T119234 since I am not going to fix the deletelogentry part right away.

For QA:

  • Create topic then immediately delete topic
    • Anonymous user should be able to view topic title, but not see content
  • Create topic then immediately suppress topic
    • Anonymous user (and any user without suppress) should not be able to view anything. It should only go in Special:Log/suppress, and only users with oversight/suppress should be able to see that log.
  • Create topic, delete topic, then suppress topic
    • There should be a deletion log entry that says "a topic" (instead of saying the topic title) was deleted. Neither the topic title nor any other part of the topic should be visible except to oversighters/suppressors. The topic page should not be linked. Suppression action should go in restricted Special:Log/suppress.
  • Create topic, delete post, delete topic, suppress topic
    • There should be a deletion log that says "deleted a post on a topic on <PAGE>. Only <PAGE> should be linked.
    • In addition to that, same as "Create topic, delete topic, then suppress topic"

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 10 2016, 8:58 PM

From IRC: this patch looks good to me, but I'm a bit puzzled by the fact that the versions of the log messages used when you can't see the title still link to the topic. That seems 1) relatively useless, because if you can't see the topic title you probably can't see anything once you arrive at the topic page either, and 2) a hiding place for a potential problem if/when we make topic page names nicer.

I accidentally git-reviewed this (sorry). I asked @dpatrick if there is a way to delete it (though probably too late now).

Anyway, I amended it to address @Catrope's concern. I was basically thinking:

  1. It didn't hurt to have it in there. It makes it seem like more of a real log entry.
  2. When we add topic titles to these URLs, it wouldn't go here, since we're not doing them on long pages.

But I don't think #2 is correct in this case, since we already show topic title here (except for the new cases in this patch), and #1 is not that compelling.

I also removed the post URLs for the same reason (they similarly include the topic page but not topic title as well).

I asked @Mooeypoo to review, but she had unrelated technical difficulties with Parsoid, so this still needs re-review. Maybe @Catrope or @matthiasmullie can look at it your morning, and deploy if it looks good.

I'm not sure we need to introduce another (non-existing) action for this. There already is a 'log' action (which uses 'history' defaults like all other lists, which seem to have the same permissions as the "view-topic-title" being introduced).

There's an @todo in Flow\Log\ActionFormatter about these permissions. The permission checks aren't currently happening because they're very expensive and apparently we never got around to fixing that :) We probably should (these permission checks are very inefficient)

// @todo: we should probably check if user isAllowed( <this-revision>, 'log' )
// unlike RC, Contributions, ... this one does not batch-load all Flow
// revisions & does not use the same Formatter, i18n message text, etc

@matthiasmullie and I had a quick talk about this. My understanding from that discussion:

  • If we end up using 'log' we should use it as is, then optimize it soon in a regular patch (not in a the security patch).
  • I should research and we should consider more how we want the log actions to behave. Specifically, in regards to previous log actions being hidden when a later log action occurs:
    • All kinds of deletion actions (regular, RevDel, and RevDel with "Suppress data from administrators as well as others") are logged.
    • If it's deleted at one of the lower levels (e.g. regular RevDel), then one of the higher levels (RevDel with hide from admins), who can view the earlier log action? So far, it doesn't seem like suppression of a revision affects visibility of earlier log actions.
    • If a revision is deleted, then the page is deleted, who can view the earlier log action? Similar to above, doesn't seem the later deletion affects visibility.

So I'm not 100% definite, but it's looking like we should get rid of the 'log' action, hide the topic title appropriately as the patch does, and fix T137288: Applying deletelogentry restrictions to flow deletion log entries does not work (which looks simple).

dpatrick triaged this task as Normal priority.Jun 14 2016, 8:10 PM
dpatrick added a project: Vuln-Infoleak.

We talked about this in the Security triage today. This one is "normal" as far as the Security team is concerned (see mw:Wikimedia_Security_Team/Prioritization_of_bugs). The assumption from that conversation is that the Collaboration team is going to handle this issue, and you all are handling it as part of your normal process for dealing with issues discovered in production.

We talked about this in the Security triage today. This one is "normal" as far as the Security team is concerned (see mw:Wikimedia_Security_Team/Prioritization_of_bugs). The assumption from that conversation is that the Collaboration team is going to handle this issue, and you all are handling it as part of your normal process for dealing with issues discovered in production.

That's correct. We have a patch, but I am working on a better solution consistent with my post above.

Yeah, I still don't think we need 'log' (the approach of auto-hiding entire log entries that were previously visible does not seem consistent with core), so I made a minor update to the patch to remove it (and fixed deletelogentry in T137288: Applying deletelogentry restrictions to flow deletion log entries does not work).

I think this is the right approach.

I would've preferred to use 'log' (similar to how our permissions work everywhere else), but than we wouldn't be able to match core log permission handling. So that patch looks good :)

Updated patch, rebased onto the patch for T137288 because they conflicted with each other.

dpatrick added a comment.EditedJun 24 2016, 9:05 PM

@Catrope deployed this on 2016-06-23:

19:24 greg-g: 19:21 < RoanKatto>  !log Synced patches for T137288 and T137593
dpatrick closed this task as Resolved.Jul 18 2016, 10:49 PM
dpatrick claimed this task.

This has been merged to master as https://gerrit.wikimedia.org/r/#/c/299864/ .

Can this task be made public?

Yes, this can be made public.

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 10 2016, 9:12 PM
demon changed Security from Software security bug to None.
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptAug 10 2016, 9:12 PM
Restricted Application added a subscriber: Malyacko. · View Herald Transcript