Page MenuHomePhabricator
Differential D10503

Check editor protocol from raw user preference
AbandonedPublic
Actions

Authored by chad on Sep 16 2014, 4:03 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 3, 5:16 AM
Unknown Object (File)
Thu, May 2, 12:47 AM
Unknown Object (File)
Thu, Apr 25, 12:50 AM
Unknown Object (File)
Sun, Apr 21, 2:08 AM
Unknown Object (File)
Sun, Apr 21, 1:57 AM
Unknown Object (File)
Sun, Apr 21, 1:38 AM
Unknown Object (File)
Mon, Apr 15, 1:52 PM
Unknown Object (File)
Mar 1 2024, 2:06 PM
View All Files
Subscribers
aik099
epriestley
Korvin

Details

Reviewers
epriestley
btrahan
Maniphest Tasks
T6106: Unable to Open in External Editor when using custom protocols
Summary

Fixes T6106, sends in the full editor URI to check if it's allowed or not

Test Plan

Set custom editor, check edit link in Diffusion and verify I don't get the help page.

Diff Detail

Repository
rP Phabricator
Branch
protocol
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 2551
Build 2555: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

chad updated this revision to Diff 25237.Sep 16 2014, 4:03 PM
chad retitled this revision from to Check editor protocol from raw user preference.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added reviewers: epriestley, btrahan.
chad added a task: T6106: Unable to Open in External Editor when using custom protocols.
Herald added subscribers: Korvin, epriestley. · View Herald TranscriptSep 16 2014, 4:03 PM
Harbormaster completed remote builds in B2551: Diff 25237.Sep 16 2014, 4:03 PM
epriestley edited edge metadata.Sep 16 2014, 4:09 PM

I don't think this is correct, and it is potentially dangerous. In theory, it allows specification of a URI like %r://evil, which might pass the hasAllowedProtocol() check but actually be dangerous of %r converts to javascript.

Can you explain the issue in more detail?

aik099 added a subscriber: aik099.Sep 16 2014, 4:11 PM
aik099 added inline comments.
src/applications/people/storage/PhabricatorUser.php
449

Does this change make any difference regardless the protocol checking? Both $uri and $editor have some protocol.

449

have same protocol

chad added a comment.Sep 16 2014, 4:19 PM

My assumption was this check was missed in D8551, but maybe I misread. What's the correct fix?

epriestley added a comment.Sep 18 2014, 3:17 PM

I believe D10515 is the real fix.

chad abandoned this revision.Sep 18 2014, 3:25 PM

Revision Contents
Changeset List

PathSize
src/
applications/
people/
storage/
2 lines

Diff 25237

View Options

src/applications/people/storage/PhabricatorUser.php

Loading...
Phabricator · Built by Phacility