Page MenuHomePhabricator

Require HTTPS for external survey link
Closed, ResolvedPublic2 Estimated Story Points

Description

What

Currently when configuring an external survey http URLs are accepted. They should be rejected when parsing the survey configuration and fail loudly.

Background

It looks like warning/erroring on http URLs was intended to become the default, but never quite implemented fully.

Some code to support this validation is still present and the frontend part is commented out.

Acceptance criteria

Event Timeline

Restricted Application added a subscriber: Aklapper. ยท View Herald TranscriptJun 12 2020, 5:19 PM

Change 605285 had a related patch set uploaded (by Awight; owner: Awight):
[operations/mediawiki-config@master] Remove unused field

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

Change 605285 merged by jenkins-bot:
[operations/mediawiki-config@master] Remove unused field

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

Change 605296 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/QuickSurveys@master] [DNM] Enforce external surveys are HTTPS

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

It seems like the surveys are already classified in the backend and there was intention to show a warning in the frontend but the code is disabled with a TODO.

https://codesearch.wmcloud.org/search/?q=isInsecure&i=nope&files=&excludeFiles=&repos=

Change 605296 abandoned by Jdlrobson:

[mediawiki/extensions/QuickSurveys@master] [DNM] Enforce external surveys are HTTPS

Reason:

No activity since June 2020.

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

We've chatted about this and I've updated the description, hopefully it is clearer and actionable now for folks with less context.

ARamirez_WMF set the point value for this task to 2.Oct 6 2021, 2:55 PM

Change 736301 had a related patch set uploaded (by Eigyan; author: Eigyan):

[mediawiki/extensions/QuickSurveys@master] WIP: Adding test case.

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

Change 736301 abandoned by Eigyan:

[mediawiki/extensions/QuickSurveys@master] WIP: Adding test case.

Reason:

Duplicate

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

Change 736300 had a related patch set uploaded (by Jhernandez; author: Eigyan):

[mediawiki/extensions/QuickSurveys@master] WIP: Adding test case.

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

Change 736301 restored by Eigyan:

[mediawiki/extensions/QuickSurveys@master] WIP: Adding test case.

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

Change 736300 abandoned by Eigyan:

[mediawiki/extensions/QuickSurveys@master] WIP: Adding test case.

Reason:

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

@eigyan I think this should be moved back to Review/Feedback?

Change 736301 merged by jenkins-bot:

[mediawiki/extensions/QuickSurveys@master] QuickSurveys: Require https for external surveys.

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

@ERayfield When you +2 a patch, we go back to the task it links to (if any) and check if it can be moved to the QA or signoff columns depending on the task. This one for example is just a tech task so it can be moved to Signoff directly skipping QA.

In other tasks where QA is next, the reviewer and owner should ensure the QA steps/instructions are up to date before moving it to that column.

Madalina added a subscriber: Madalina.

Marking this as resolved