Summary:
I didn't realize createDiffDict was a public method when I
modified it, and I broke the API call in getrevision. This moves the
modification inside the method and reverts the method header back to
it's original form.
Test Plan: none
Reviewers: epriestley
Reviewed By: epriestley
CC: edward, aran, epriestley
Differential Revision: 883
Summary:
After D857, we try to attach local commit information to revisions. If this
information is available, display it on the revision.
Design on this is a little rough, I might try to combine this into the revision
update view or something like that since we're starting to take up a lot of real
estate for metadata.
Test Plan: Local diffed this and got some commit info.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 872
Summary:
Provide three Phriction methods for programmatic access to Phriction:
- phriction.info: get information about a document
- phriction.history: get change history for a document
- phriction.edit: create or update a document
I moved all the editing/creation logic into a new PhrictionDocumentEditor to
share code between the Conduit and Web edit pathways.
Test Plan: Got info and history via conduit. Edited and created new pages via
conduit and web.
Reviewers: hsb, jungejason, tuomaspelkonen, aran, hunterbridges
Reviewed By: hunterbridges
CC: skrul, aran, hunterbridges
Differential Revision: 866
Summary:
We need createlintresults because we are doing extended
static analysis offline, and thus we need to be able to update the
lint results associated with a diff. This is similar to
updateunitresults, but "create" is more accurate than "update" since
we never need to change existing lint results.
getdiffproperty is used by the client to ensure it isn't creating any
duplicates lint results. It's the symmetric operation to
setdiffproperty, which already exists.
Test Plan:
We have a new offline linter that I used to test. This
linter calls getdiffproperty on every run.
1. Tested updating an existing set of lint results by first running
"arc diff" with lint errors caught by the local linter, then later
running offline analysis which catches one other error and updates via
createlintresults. Ensured the differential lint results were as
expected.
2. Tested the creation of an entirely new diff property through
createlintresults. I first ran "arc diff --nolint" to skip all lint
results, then ran offline analysis which caught an error and updated
through createlintresults. Ensured differential lint results were as
expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: dpepper, aran, mgummelt, jungejason, epriestley
Differential Revision: 868
Summary: Oops, I left this in from an earlier version and missed it since I was
mostly looking at Maniphest for testing. We already render this information in
the header, don't additionally render it under the comments.
Test Plan: derp derp, loaded any revision with sourced comments
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 871
Summary: Execute searches like the primary Maniphest task list. Let me know what
else you guys need from this API.
Test Plan: Executed "maniphest.info" and "maniphest.find"
Reviewers: jungejason, tuomaspelkonen, aran, nh
Reviewed By: nh
CC: blair, skrul, aran, jungejason, epriestley, nh, tuomaspelkonen
Differential Revision: 867
Summary:
When an object is updated, record the content source for the update. This mostly
isn't terribly useful but one concrete thing I want to do with it is let admins
audit via-email replies more easily since there are a bunch of options which let
you do hyjinx if you intentionally configure them insecurely. I think having a
little more auditability around this feature is generally good. At some point
I'm going to turn this into a link admins can click to see details.
It also allows us to see how frequently different mechanisms are used, and lets
you see if someone is at their desk or on a mobile or whatever, at least
indirectly.
The "tablet" and "mobile" sources are currently unused but I figured I'd throw
them in anyway. SMS support should definitely happen at some point.
Not 100% sure about the design for this, I might change it to plain text at some
point.
Test Plan: Updated objects and saw update sources rendered.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 844
Summary: When a user stores the empty string in an auxiliary field, simply don't
store it, and delete it if it already exists.
Test Plan: Edited a revision with an empty "Quack" field, got an empty row in
the DB. Applied patch, edited empty again, row went away. Edited empty again,
still no row. Edited and put something in the field, got a row.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 865
Summary:
It is possible to view a comment that has no cache; when viewing such a comment
the request doesn't have a csrf token and there is no need for one, so we turn
off the write guard.
Test Plan:
loaded an old diff that had no cache, and the page loaded instead of throwing
an AphrontCSRFException.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 858
Summary: Quora wants to handle some moderation tasks with Phabricator, but want
to lower the barrier to entry for the install and let moderators adopt it
gradually. One request is to allow auth rules to be relaxed so we can auth based
on Reply-To to make things easier. This is insecure if configured but not really
a big deal and the patch isn't big or complicated.
Test Plan: Sent a test email with bogus "From" but valid "Reply-To". It was
rejected with this setting off, and allowed with this setting on.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 842
Summary: Delete one line which has no effect.
Test Plan: Open revision page to make sure it still works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 852
Summary: Instead of just saying a task is "Closed", say "Resolved", "Wontfix",
etc.
Test Plan: Looked at task list view, saw "Resolved", "Wontfix", etc.
Reviewers: skrul, hunterbridges, jungejason, tuomaspelkonen, aran
Reviewed By: skrul
CC: aran, skrul, jungejason, epriestley
Differential Revision: 851
Summary: If a user partially discovers a repository and then deletes it, the
timeline will have events from the old repository which this daemon won't be
able to parse.
Test Plan: @ajtrichards, can you apply this locally and restart your daemons
(##phd stop##, then relaunch them) and let me know if it fixes the issue?
Reviewers: ajtrichards, jungejason, tuomaspelkonen, aran
Reviewed By: ajtrichards
CC: aran, epriestley, ajtrichards
Differential Revision: 845
Summary:
When selecting children of a directory, it is possible that none of its
children exist anymore even though the directory still exists. After fetching
the children but before returning them, we should check whether there are any,
and if there are no children, set the reason as empty directory.
Test Plan:
In sandbox, browsed in diffusion to a directory that exists but has no
files and saw that it has a useful message instead of a vague exception.
Reviewers: epriestley, tuomaspelkonen, jungejason
Reviewed By: tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 846
Summary:
This allows you to configure a single mailbox for all mail sent by phabricator,
so you
can keep a mailaddress like bugs@example.com and don't need a catchall on your
domain/subdomain.
Test Plan:
Enabled and disabled suffix. Saw mails generated have to correct prefix. Also
piped raw mails
into the scripts/mail/mail_handler.php and ensured comments went into
phabricator for both maniphest
and differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 815
Summary: This could be a lot fancier but let's see what else we need. Also fixed
some bugs with maniphest.info.
Test Plan: Used the Conduit web console to create some tasks with different
values.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 824
Summary: Open AphrontWriteGuard for user login.
Test Plan: verified that the user can log in.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 840
Summary: When a user has bad cookies, try to clear everything and tell them they
might need to manually clear things.
Test Plan: Added "&& false" to the valid branch and got the exception message.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 839
Summary:
After D814 and D829, you should be able to implement this logic in the
didWriteRevision() method of the field.
Note that the attacher is still referenced in
ConduitAPI_differential_updatetaskrevisionassoc_Method. This method should
probably be moved to facebook/ since it's pretty Facebook-specific.
No rush on any of this, it's not hurting anything.
Test Plan:
- Hit differential.getcommitmessage
- Ran 'arc diff'
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 830
Summary:
In D758, I tightened the scope for which we issue cookies. Instead of setting
them on the whole domain we set them only on the subdomain, and we set them as
HTTPS only if the install is HTTPS.
However, this can leave the user with a stale HTTP cookie which the browser
sends and which never gets cleared. Handle this situation by:
- Clear all four <domain, https> pairs when clearing cookies ("nuke it from
orbit").
- Clear 'phsid' cookies when they're invalid.
Test Plan: Applied a hackier version of this patch to secure.phabricator.com and
was able to login with a stale HTTP cookie.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 838
Summary:
This commit lets users who are filing Maniphest tasks to attache files to them
right off the bat.
Test Plan:
{F3545}
and
{F3546}
Reviewers: epriestley, fmoo, aran
CC:
Differential Revision: 837
Summary:
Remove the blame revision, revert plan and lines fields from the default field
loadout. (After D829 this doesn't cause issues where we have bogus dictionary
entries.)
You should add these back to the Facebook configuration since Facebook wants
these fields. However, I want to keep the default stack very light and I never
saw a huge amount of value in these fields at Facebook so I don't think they
make the cut. Sorry, tomo. ;_;
Test Plan: Ran "arc diff" locally.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, tomo, epriestley
Differential Revision: 831
Differential comments
Summary: If you @mention several users, at least one of which is already CC'd,
we unset all the CCs and don't attach the "Added CCs: ..." block to the comment.
Test Plan: @mentioned two users, one of whom was already CC'd.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 827
Summary:
I think this is the last major step -- use the fields to parse commit messages,
not a hard-coded list of stuff. This adds two primary methods to fields, one to
get all the labels they'll parse (so we can do "CC" and "CCs" and treat them as
the same field) and one to parse the string into a canonical representation
(e.g., lookup reviewers and such).
You'll need to impelement the one block of task-specific stuff I removed in
Facebook's task field:
list($pre_comment) = split(' -- ', $data);
$data = array_filter(preg_split('/[^\d]+/', $pre_comment));
foreach ($data as $k => $v) {
$data[$k] = (int)$v;
}
$data = array_unique($data);
break;
Otherwise I think this is clean.
Test Plan:
- Called the conduit method with various commit messages, parsed fields/errors
seemed correct.
- "arc diff"'d this diff onto localhost, then updated it.
- "arc amend"'d this diff.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 829
Summary: While I thought this was complicated, there was nothing subtle or
tricky here -- I just misnamed a variable.
Test Plan: Created a revision with default CCs, got CCs instead of nothing.
Reviewers: aran, jungejason, tuomaspelkonen
Reviewed By: aran
CC: aran
Differential Revision: 834
Summary:
deprecate generateProperties() from class
DifferentialRevisionDetailRenderer. Custom fields now provides a much
more powerful version of generateProperties().
Depends on D814.
Test Plan:
implemented facebook task field with custom field and
verified it worked.
Reviewers: epriestley, tuomaspelkonen
Reviewed By: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 826
Summary:
When rendering commit messages, drive all the logic through field specification
classes instead of the hard-coded DifferentialCommitMessageData class. This
removes DifferentialCommitMessageData and support classes.
Note that this effectively reverts D546, and will cause a minor break for
Facebook (Task IDs will no longer render in commit messages generated by "arc
amend", and will not be editable via "arc diff --edit"). This can be resolved by
implementing the feature as a custom field. While I've been able to preserve the
task ID functionality elsewhere, I felt this implementation was too complex to
reasonably leave hooks for, and the break is pretty minor.
Test Plan:
- Made numerous calls to differential.getcommitmessage across many diffs in
various states, with and without 'edit' and with and without various field
overrides.
- General behavior seems correct (messages look accurate, and have the
expected information). Special fields like "Reviewed By" and "git-svn-id" seem
to work correctly.
- Edit behavior seems correct (edit mode shows all editable fields, hides
fields like "Reviewed By").
- Field overwrite behavior seems correct (overwritable fields show the correct
values when overwritten, ignore provided values otherwise).
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 814
Summary:
See T354. List every rule which has ever been applied in X-Herald-Rules, not
just the ones which most recently triggered.
Also some random fixes while I was debugging this:
- When conduit methods throw non-conduit exceptions, make sure they get
logged.
- Trigger the Facebook "tasks" backcompat block only if we were going to fail
(this should reduce the shakniess of the transition).
- Fix some log spew from the new field stuff.
Test Plan:
- Created a rule (ID #3) "No Zebras" which triggers for revisions without
"zebra" in the title.
- Created a revision without "zebra" in the title, got X-Herald-Rules: <2>,
<3>
- Updated revision to have "zebra" in the title, verified rule did not trigger
in Herald transcript.
- Verified X-Herald-Rules is still: <2>, <3>
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 817
Summary:
Provide a catchall mechanism to find unprotected writes.
- Depends on D758.
- Similar to WriteOnHTTPGet stuff from Facebook's stack.
- Since we have a small number of storage mechanisms and highly structured
read/write pathways, we can explicitly answer the question "is this page
performing a write?".
- Never allow writes without CSRF checks.
- This will probably break some things. That's fine: they're CSRF
vulnerabilities or weird edge cases that we can fix. But don't push to Facebook
for a few days unless you're prepared to deal with this.
- **>>> MEGADERP: All Conduit write APIs are currently vulnerable to CSRF!
<<<**
Test Plan:
- Ran some scripts that perform writes (scripts/search indexers), no issues.
- Performed normal CSRF submits.
- Added writes to an un-CSRF'd page, got an exception.
- Executed conduit methods.
- Did login/logout (this works because the logged-out user validates the
logged-out csrf "token").
- Did OAuth login.
- Did OAuth registration.
Reviewers: pedram, andrewjcg, erling, jungejason, tuomaspelkonen, aran,
codeblock
Commenters: pedram
CC: aran, epriestley, pedram
Differential Revision: 777
domain
Summary:
See D758, D759.
- Provide a strongly recommended setting which permits configuration of an
alternate domain.
- Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
- Prevent Phabriator from setting cookies on other domains.
This assumes D759 will land, it is not effective without that change.
Test Plan:
- Attempted to login from a different domain and was rejected.
- Logged out, logged back in normally.
- Put install in setup mode and verified it revealed a warning.
- Configured an alterate domain.
- Tried to view an image with an old URI, got a 400.
- Went to /files/ and verified links rendered to the alternate domain.
- Viewed an alternate domain file.
- Tried to view an alternate domain file without the secret key, got a 404.
Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760
Summary: This prevents <applet /> attacks unless the attacker can upload an
applet which has a viewable MIME type as detected by `file`. I'm not sure if
this is possible or not. It should, at least, narrow the attack window. There
are no real tradeoffs here, this is probably a strictly better application
behavior regardless of the security issues.
Test Plan:
- Tried to download a file via GET, got redirected to info.
- Downloaded a file via POST + CSRF from the info page.
Reviewers: andrewjcg, erling, aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 759
Summary:
We don't currently validate CSRF tokens on this workflow. This allows an
attacker to upload arbitrary files on the user's behalf. Although I believe the
tight list of servable mime-types means that's more or less the end of the
attack, this is still a vulnerability.
In the long term, the right solution is probably to pass CSRF tokens on all Ajax
requests in an HTTP header (or just a GET param) or something like that.
However, this endpoint is unique and this is the quickest and most direct way to
close the hole.
Test Plan:
- Drop-uploaded files to Files, Maniphest, Phriction and Differential.
- Modified CSRF vaidator to use __csrf__.'x' and verified uploads and form
submissions don't work.
Reviewers: andrewjcg, aran, jungejason, tuomaspelkonen, erling
Commenters: andrewjcg, pedram
CC: aran, epriestley, andrewjcg, pedram
Differential Revision: 758
Summary: See T429. When you hit certain errors, you get less-than-helpful
messages like "upload error 3". Instead, produce human-readable errors.
Test Plan: Simulated errors, verified user receives decent error messages.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran, startupguy
CC: aran
Differential Revision: 816
Summary:
When we create or update a revision, we use a parsed commit message dictionary
to edit its fields. Drive consumption of the dictionary through custom fields
instead of hardcoding.
This requires adding some fields which don't really do anything right now to
cover fields which appear only in the commit message.
Test Plan: "arc diff"'d this revision against localhost, "arc diff"'d again to
update.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 811
