Summary:
@vrana patched an important external-CSRF-leaking hole recently (D1558), but
since we are sloppy in building this form it got caught in the crossfire.
We set action to something like "http://this.server.com/oauth/derp/", but that
triggers CSRF protection by removing CSRF tokens from the form. This makes OAuth
login not work.
Instead, use the local path only so we generate a CSRF token.
Test Plan: Registered locally via oauth.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley, demo
Maniphest Tasks: T853
Differential Revision: https://secure.phabricator.com/D1597
Summary: Rough cut for Quora, we want this too eventually but it's super basic
right now so I'm not linking it anywhere. Once we get a couple more iterations
I'll put it in the UI.
Test Plan: Looked at stats for test data.
Reviewers: btrahan
Reviewed By: btrahan
CC: anjali, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1594
Summary: Looping on this interface is pretty useful but you don't always want to
keep the projects/owners.
Test Plan: Clicked both buttons.
Reviewers: btrahan
Reviewed By: btrahan
CC: anjali, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1593
Summary:
Rough cut that still needs a lot of polish, but replace the directory list with
more of a dashboard type thing:
- Show "Unbreak Now", triage-in-your-projects, and other stuff that you're
supposed to deal with, then feed.
- Move tools a click a way behind nav -- this also lets us put more stuff
there and subtools, etc., later.
- Remove tabs.
- Merge the category/item editing views.
- I also added a light blue wash to the side nav, not sure if I like that or
not.
Test Plan:
- Viewed all elements in empty and nonempty states.
- Viewed applications, edited items/categories.
Reviewers: btrahan, aran
Reviewed By: btrahan
CC: aran, epriestley, davidreuss
Maniphest Tasks: T21, T631
Differential Revision: https://secure.phabricator.com/D1574
Summary:
This was a sort of speculative feature added by a contributor some time ago and
just serves as a label; for now, simplify it into "active" and "archived" and
remove "archived" projects from the "active" list.
- Fix a bug where we'd publish a "renamed from X to X" transaction that had no
effect.
- Publish stories about status changes.
- Remove the "edit affiliation" controller, which has no links in the UI
(effectively replaced by join/leave links).
- Add query/conduit support.
Test Plan: Edited the status of several projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T681
Differential Revision: https://secure.phabricator.com/D1573
Summary:
We save search information and then redirect to a "/search/<query_id>/" URI in
order to make search URIs short and bookmarkable, and save query data for
analysis/improvement of search results.
Currently, there's a vague object enumeration security issue with using
sequential IDs to identify searches, where non-admins can see searches other
users have performed. This isn't really too concerning but we lose nothing by
using random keys from a large ID space instead.
- Drop 'authorPHID', which was unused anyway, so searches can not be
personally identified, even by admins.
- Identify searches by random hash keys, not sequential IDs.
- Map old queries' keys to their IDs so we don't break any existing bookmarked
URIs.
Test Plan: Ran several searches, got redirected to URIs with random hashes from
a large ID space rather than sequential integers.
Reviewers: arice, btrahan
Reviewed By: arice
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1587
Summary:
In D1515, I introduced some excessively-complicated semantics for detecting
connections that are lost while transactional. These semantics cause us to
reenter establishConnection() and establish twice as many connections as we need
in the common case.
We don't need a hook there at all -- it's sufficient to throw the exception
rather than retrying the query when we encounter it. This doesn't have
reentrancy problems.
Test Plan:
- Added some encapsulation-violating hooks and a unit test for them
- Verified we no longer double-connect.
Reviewers: btrahan, nh
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T835
Differential Revision: https://secure.phabricator.com/D1576
Summary:
This got caught in the crossfire when we admin-only'd the whole MetaMTA tool. It
should not be admin only.
(Generally, we should probably separate this out better at some point.)
Test Plan: Hit /mail/sendgrid/ as a logged-out, non-admin user (like SendGrid
does).
Reviewers: s, btrahan
Reviewed By: s
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1588
Summary:
The main purpose of this change is to allow selecting the branch by
triple-click.
Plus it is not perfectly clear that the text in brackets means branch.
Test Plan: Display revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1585
Summary:
See discussion in T838. These fields expose information which it isn't necessary
or useful to expose in the general case.
- Disable fields by default, allow them to be enabled in config (these fields
were useful for me at Facebook when I had access to all the machines).
- Remove 'sourcePath' from Conduit methods other than differential.query.
- Condition 'sourcePath' field in Conduit on the caller being the revision
author. This is a bit hacky but not so awful.
Test Plan:
- Verified fields are gone by default and restored by configuration.
- Verified Conduit no longer returns these fields other than
differential.query.
- Verified field presence/absence according to authorship in
differential.query.
- Grepped around in arcanist to make sure we aren't relying on sourcePath.
There's a workflow in "arc merge" that technically might hit it, but I think
it's unreachable, definitely irrelvant (we never use source path as a
distinguisher under git/hg, and can't 'arc merge' in SVN) and it's going away
Real Soon Now anyway.
Reviewers: btrahan, arice
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T838
Differential Revision: https://secure.phabricator.com/D1582
Summary:
See D1533#5.
Also deduplicates logic of what is stored to blob in ArcanistDiffWorkflow.
Blame Rev: D1533
Test Plan:
Display raw version of text file.
Display raw version of image.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1583
Summary:
Escaped $id is compared with non-escaped $max_id.
Escaped $id is escaped again in phutil_render_tag().
Note: $id is numeric :-).
Test Plan: Display diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1580
Summary:
This addresses a few things:
- Provide a software HTTP response spliting guard as an extra layer of
security, see http://news.php.net/php.internals/57655 and who knows what HPHP/i
does.
- Cleans up webroot/index.php a little bit, I want to get that file under
control eventually.
- Eventually I want to collect bytes in/out metrics and this allows us to do
that easily.
- We may eventually want to write to a socket or do something else like that,
ala Litespawn.
Test Plan:
- Ran unit tests.
- Browsed around, checked headers and HTTP status codes.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1564
Summary: I've chosen passing callsign even if it is more complicated because it
is nicer than PHID and can be written by hand.
Test Plan:
Search without repository.
Search with repository.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1571
Summary:
This just looks silly:
{F8088, size=full}
It runs in O(N*N) but it's not a big deal because there are usually only few
comments per line.
I didn't implement it for images.
Test Plan:
View revision with compatible inline comments in two diffs.
View revision with different inline comments on same line in two diffs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1570
Summary: Remarkup object names require #1 for linking to comments which is not
very intuitive.
Test Plan:
D1558#4e01328c
D1558#1
D1558#comment-1
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1565
Summary:
This code was just all kinds of wrong, but got all the common cases anyone cares
about correct.
- In edit-inline-comments.js, if isOnRight() is true, use data.right, not
data.left (derp).
- Set data.left correctly, not to the same value as data.right (derp derp).
- Set "isNewFile" based on $is_new, not $on_right (derp derp derp).
Test Plan:
- Added JS debugging code to print "OLD" vs "NEW" and "LEFT" vs "RIGHT".
Clicked the left and right sides of diff-vs-base and diff-vs-diff diffs,
verified output was accurate in all cases.
- Added comments to the left-display-side of a diff-of-diffs, saved them, they
showed up where I put them.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T543
Differential Revision: https://secure.phabricator.com/D1567
Summary:
add a static variable to the method and use it so we don't init more
than once!
Test Plan:
add a "phlog" and noted only init'd one time. verified "show more"
links worked correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T666
Differential Revision: https://secure.phabricator.com/D1553
Summary:
Sending CSRF token in GET forms is dangerous because if there are external links
on the target page then the token could leak through Referer header.
The token is not required for anything because GET forms are used only to
display data, not to perform operations.
Sending CSRF tokens to external URLs leaks the token immediately.
Please note that <form action> defaults to GET.
PhabricatorUserOAuthSettingsPanelController suffered from this problem for both
reasons.
Test Plan: Save my settings (POST form).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1558
Summary: Phabricator sends information about encoding in Content-Type header but
when I save the HTML page then this information is lost.
Test Plan: /
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1561
Summary:
The form doesn't perform any action, only displays data.
URL wouldn't exceed its maximum length.
Bookmarking results can be useful.
Linking from other pages can be even more useful.
Also browsing through history is more fluent.
Test Plan:
Search by path.
Search by owner.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1559
Summary:
Search tool currently allows only searching by substring which is useful.
But searching by the full path (for packages in which the path is contained) is
probably even more useful.
NOTE: I used a trick to perform the search by superstring.
Packages containing path '/' are found always which could seem strange but it is
correct after all.
Test Plan:
Search for /src/x/a.php. Found package with path /src/x/.
Search for /src/. Found package with path /src/x/.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1560
Summary: Also changes rCALL in package detail to bold CALL.
Test Plan:
/owners/view/all/, click on link
/owners/package/1/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1550
Summary: This is not totally done yet, and i'm submitting for feedback.
Test Plan: Played with various settings in local conduit console.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T790
Differential Revision: https://secure.phabricator.com/D1555
Summary: This exposes a few remarkup engines over conduit.
Test Plan:
Local conduit console, and playing with
'cat example.json | arc call-conduit remarkup.process'
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1551
Summary:
@rguerin ran into an issue in his install where Phabricator appears to have
discovered commits which no longer exist, and thus is failing to proceed with
its repository import.
It's not clear how we got into this state. Previously, it was possible by, e.g.,
parsing a different repository's working copy and then switching them back, but
there are now safeguards against that.
I'm taking a three-pronged approach to try to sort this out:
- Provide a script to get out of this state (this script) and reconcile
Phabricator's view of a repository with an authoritative copy of it. This
basically "un-discovers" any discovered commits which don't actually exist (any
queued tasks to parse them will fail permanently when they fail to load the
commit object).
- Add more logging to the discovery daemon so we can figure out where commits
came from.
- Improve Diffusion's UI when stuff is partially discovered (T776).
(This script should also clean up some nonsense on secure.phabricator.com from a
botched Diviner import.)
Test Plan: Ran "reconcile.php" with bogus commits and bogus differential/commit
links, had them expunged. Will work with @rguerin to see if this resolves
things.
Reviewers: btrahan, rguerin
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1552
Summary:
This doesn't cover every case exhaustively (see comments) but should cover like
98% of the practical cases.
This makes one workflow modification: willWriteRevision() was previously
guaranteed to have a revisionID / revisionPHID and no longer is. I verified that
no field implementations depend on this behavior. Fields which depend on IDs
should be using didWriteRevision() instead.
Test Plan: Inserted a "throw" into the middle of the transactions and created
revisions; they didn't orphan. Created revisions normally, they worked
correctly.
Reviewers: btrahan, nh
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T605
Differential Revision: https://secure.phabricator.com/D1541
Summary: posix may not be loaded on the web/cgi SAPI but we call posix functions
on this pathway, which we hit on /daemon/. Fall back to exec if we don't have
posix.
Test Plan: Added "&& false" and verified the page executed a bunch of "ps"
tests.
Reviewers: Koolvin, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T821
Differential Revision: https://secure.phabricator.com/D1540
Summary:
conduit was using getProductionURI instead of getURI for checking that the
request was sent to the correct host, which causes problems in some dev
environments
Test Plan:
echo '{}' | arc call-conduit --conduit-uri=mydevserver
where my dev server is configured with phabricator.production-uri pointing to
prod instead of my devserver
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1543
Summary: I think that these are the only links that are useful - commit which
deleted the path and last version of the path.
Test Plan: Display deleted path, click on links.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1536
Summary:
When a user selects "show raw file (right)" from the dropdown of a binary file
in differential, they should get more than a blank page.
Test Plan: Loaded a raw binary file from differential
Reviewers: tuomaspelkonen, epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1533
Summary: Since mailing list rules are now "global", don't run "personal" rules
for disabled/invalid users.
Test Plan: Added a personal rule that matches every revision for a test user.
Created a revision, checked transcript, rule matched. Disabled user, updated
revision, checked transcript, rule got auto-disabled.
Reviewers: btrahan, jungejason, nh, xela
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1517
Summary:
- Default "personal" vs "global" choice to "personal".
- Don't show global rules under "My Rules".
- After editing or creating a global rule, redirect back to global rule list.
- Use radio buttons for "personal" vs "global" and add captions explaining the
difference.
- For "global" rules, don't show the owner/author in the rule detail view --
they effectively have no owner (see also D1387).
- For "global" rules, don't show the owner/author in the rule list view, as
above.
- For admin views, show rule type (global vs personal).
Test Plan:
- Created and edited new global and personal rules.
- Viewed "my", "global" and "admin" views.
Reviewers: btrahan, jungejason, nh, xela
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1518
Summary: Provide explicit guidance in the documentation about liberal use of
"final".
Test Plan: Generated, read documentation.
Reviewers: btrahan, jungejason, aran, nh
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1520
Summary:
Render coverage information in the right gutter, if available.
We could render some kind of summary report deal too but this seems like a good
start.
Test Plan:
- Looked at diffs with coverage.
- Looked at diffs without coverage.
- Used inline comments, diff-of-diff, "show more", "show entire file", "show
generated file", "undo". Nothing seemed disrupted by the addition of a 5th
column.
Reviewers: btrahan, tuomaspelkonen, jungejason
Reviewed By: btrahan
CC: zeeg, aran, epriestley
Maniphest Tasks: T140
Differential Revision: https://secure.phabricator.com/D1527
Summary: We were not correctly updating $diff as we iterated through the loop.
Test Plan: Viewed a revision several diffs that had differing base revision.
Reviewers: fratrik, btrahan
Reviewed By: fratrik
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1523
Summary:
Restores a (simplified and improved) version of Lisk transactions.
This doesn't actually use transactions anywhere yet. DifferentialRevisionEditor
is the #1 (and only?) case where we have transaction problems right now, but
sticking save() inside a transaction unconditionally will leave us holding a
transaction open for like a million years while we run Herald rules, etc. I want
to do some refactoring there separately from this diff before making it
transactional.
NOTE: @jungejason / @nh, can one of you verify these unit tests pass on
HPHP/i/vm when you get a chance? I vaguely recall there was some problem with
(int)$resource. We can land this safely without verifying that, but should check
before we start using it anywhere.
Test Plan: Ran unit tests.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T605
Differential Revision: https://secure.phabricator.com/D1515
