Page MenuHomePhabricator
Create Task
Maniphest T140129

VE should always forward cookies when $wgVisualEditorParsoidForwardCookies is set
Closed, ResolvedPublic1 Estimated Story Points
Actions

Description

ApiVisualEditor.php ignores $wgVisualEditorParsoidForwardCookies if it thinks the wiki is publically readable.

@Catrope's initial commit didn't include this, but the check if the wiki was publicly readable was added by the time the patch was accepted. The rationale given ( Oct 23, 2013) was

Patch Set 2: Code-Review-1

Per discussion with Gabriel on IRC, this feature should refuse to work if there are no read restrictions. Will amend.

However, this ("refuse to work if there are no read restrictions") isn't documented anywhere and is counter-intuitive. The wiki I run is behind CA's SSO provider, for example, so it is unreadable without authentication no matter what the internal setting of MediaWiki is.

At the very least this should be better documented -- "If you set $wgVisualEditorParsoidForwardCookies make sure that you also set your wiki so that it is not publicly readable" -- but I think it would be better to honor the setting in all cases.

There are already a lot of disclaimers on the documentation for this, so we should assume that if someone sets it, they know what they're doing.

Details

Related Changes in Gerrit:
SubjectAuthorRepoBranchLines +/-
Always forward cookies when $wgVisualEditorParsoidForwardCookies is setMarkAHershbergermediawiki/extensions/VisualEditormaster+1 -1
Customize query in gerrit

Event Timeline

MarkAHershberger created this task.Jul 12 2016, 4:50 PM
Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptJul 12 2016, 4:50 PM
gerritbot subscribed.Jul 12 2016, 5:01 PM

Change 298526 had a related patch set uploaded (by MarkAHershberger):
Always forward cookies when $wgVisualEditorParsoidForwardCookies is set

https://gerrit.wikimedia.org/r/298526

gerritbot added a project: Patch-For-Review.Jul 12 2016, 5:01 PM
Jdforrester-WMF subscribed.Jul 12 2016, 7:04 PM

Hmm. I'm minded to retain this security check. @GWicke, as this was your suggestion, what do you think?

Jdforrester-WMF triaged this task as Lowest priority.Jul 12 2016, 7:05 PM
Jdforrester-WMF added projects: VisualEditor-MediaWiki, Parsoid.
Jdforrester-WMF set the point value for this task to 1.
Jdforrester-WMF moved this task from To Triage to TR0: Interrupt on the VisualEditor board.
MarkAHershberger added a comment.Jul 12 2016, 7:49 PM
In T140129#2454726, @Jdforrester-WMF wrote:

Hmm. I'm minded to retain this security check. @GWicke, as this was your suggestion, what do you think?

It isn't really a security check. It is policy enforcement of possibly irrelevant policy. As I stated there are already other controls in place for this particular wiki that this "security check" is superfluous.

If you decide not to take this patch, then plenty of warnings should be added to the documentation for this.

• GWicke added a comment.Jul 12 2016, 10:16 PM

The reason for not forwarding cookies to default-public wikis is that we do not want to cache or store specifically restricted content (ex: revision deleted pages) without also performing the requisite access checks. Doing so would circumvent content access restrictions configured on the wiki.

The wiki I run is behind CA's SSO provider, for example, so it is unreadable without authentication no matter what the internal setting of MediaWiki is.

So, is this a form of authentication that does not use MediaWiki's ACLs?

MarkAHershberger added a comment.Jul 12 2016, 11:03 PM
In T140129#2455675, @GWicke wrote:

So, is this a form of authentication that does not use MediaWiki's ACLs?

All MediaWiki ACLs except 'read' can be used normally. The SSO component is only used for authentication and authorization of 'read' access. I've configured the wiki to create users automatically based on the information from the SSO component.

Before I discovered this undocumented feature, I had the wiki configured as a public wiki (since it was publicly readable before the introduction of SSO).

MarkAHershberger added a comment.Jul 13 2016, 8:40 PM

@GWicke: would you be willing to +2 the patch now?

ssastry moved this task from Needs Triage to Non-Parsing-Team Tasks on the Parsoid board.Jul 15 2016, 9:21 PM
Jdforrester-WMF closed this task as Resolved.Aug 15 2016, 11:10 PM
gerritbot added a comment.Aug 15 2016, 11:21 PM

Change 298526 merged by jenkins-bot:
Always forward cookies when $wgVisualEditorParsoidForwardCookies is set

https://gerrit.wikimedia.org/r/298526

ReleaseTaggerBot added a project: MW-1.28-release (WMF-deploy-2016-08-16_(1.28.0-wmf.15)).Aug 16 2016, 12:01 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits