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
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
Summary:
Move all the rest of the fields into the custom field schema, for revision
views.
I left a couple of stubs in here (willWriteRevision, didWriteRevision) since I'd
planned to do edits here too, but this diff is sort of big-ish already. I'll do
all the edit fields in the next revision.
Depends on D808.
Test Plan: Viewed, edited and conduit'ed some revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 809
Summary:
Move additional fields (which rely on loading handles) to the extensible field
classes and out of hardcoding in the controller.
Depends on D807.
Test Plan: Viewed, edited, and hit conduit for revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 808
Summary:
Differential has a bunch of display-only fields, implement them all as field
specifications instead of hard-coded fields.
Also add some more documentation and fix redundant string constants in blame
rev/revert plan fields.
Test Plan: Viewed, edited, and hit conduit for revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 807
Summary: Similar to D785 for Maniphest, expose auxiliary field values via
Conduit.
Test Plan: Ran revision.getinfo on a revision with aux fields, got them in the
response.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 802
Summary:
This is just to ease transitions for any installs which use these fields (e.g.,
Facebook). I'll write some docs and a migration script once this stuff is a
little more solid, too.
Depends on D800.
Technically these are "better" than the current fields since they show up other
places than the edit screen (derp derp).
Test Plan: Created a field selector which provides these; verified they work by
typing stuff into them and saving the revision.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 801
Summary: Depends on D798. Extends custom fields and makes the vaguely useful:
they can appear on the edit and view interfaces. This does not integrate them
with commit messages yet; that's more complicated but I plan to do it shortly.
Test Plan: Implemented a custom field per P123, it correctly appears on the edit
interface, persists, validates, and shows up when viewing the revision.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 800
Summary:
Precursor to building this out to solve T343. This is similar to the Maniphest
fields we landed recently, although I think they're dissimilar enough that it
isn't worth going crazy trying to make them share code, at least for now.
This doesn't really do anything yet, just adds a storage object and a couple of
selector/field indirection classes.
Test Plan: Ran SQL upgrade script, created an aux field.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 798
Summary:
This allows you to edit dependencies. It is a better patch than it used to be.
It depends on D725.
- If you create a cycle, it just throws an exception and aborts the workflow.
It should not do this.
- Tasks which depend on the current task aren't shown in the UI. Need to add a
new table for this.
- Transaction text says "attached Task" but should probably say "added a
dependency on task".
Test Plan: Created valid and invalid dependencies between tasks. Created valid
and invalid dependencies between revisions.
Reviewed By: tuomaspelkonen
Reviewers: davidreuss, jungejason, tuomaspelkonen, aran
Commenters: codeblock
CC: aran, codeblock, tuomaspelkonen, epriestley
Differential Revision: 595
Summary:
This gets all the major pieces working. Allows you to drag-and-drop files in
Differential and Phriction, and embed files in remarkup with {Fxxx} references.
See also task.
I'm explicitly not documenting this yet since it's still pretty rough.
Test Plan: Dragged and dropped stuff into Differential and Phriction.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran, tomo
Commenters: tomo
CC: aran, tomo, jungejason
Differential Revision: 674
Summary: See T368. The current rendering result can cause some confusion for the
first/last chunks, make their behavior more explicit.
Test Plan:
- Clicked various "show more" links on a bunch of top/bottom/middle omitted
context blocks in a variety of diffs.
- Located a @generated shielded file and verified the initial render is
correct when the entire file is default-hidden.
Reviewed By: avitaloliver
Reviewers: avitaloliver, jungejason, tuomaspelkonen, aran
CC: aran, avitaloliver
Differential Revision: 744
Summary:
Python people don't seem to like the 'ignore-all' as default. Provide a way
to configure which file types should not use 'ignore-all'.
Test Plan:
Tested that it worked with bunch of Python of files and non-python
files. Cache was disabled during the test.
Reviewed By: jungejason
Reviewers: epriestley, jungejason
Commenters: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 713
Summary:
This is really rough and needs work (particularly, there's some diff code I
really need to refactor since I sort-of-copy-pasted it) but basically
functional.
Show text changes between diffs and allow users to revert to earlier versions.
Differential's line-oriented diff style isn't ideal for large blocks of text but
I'm betting this is probably good enough in most cases. We can see how bad it is
in practice and then fix it if needbe.
I added a bunch of support for "description" but didn't add the feature in this
diff, I'll either follow up or task it out since it should be a pretty
straightforward change.
Test Plan: Looked at history for several Phriction documents, clicked "previous
change" / "next change", clicked revert buttons.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen
CC: aran, hsb, epriestley
Differential Revision: 687
Summary:
Show line count, arcanist project and base revision.
This adds a little clutter but I think we're still okay and I can play around
with it later.
Test Plan: Looked at a couple of revisions. I'm actually not 100% sure about the
SVN logic but maybe I will test that before committing.
Reviewed By: tomo
Reviewers: tomo, jungejason, tuomaspelkonen, aran
CC: aran, tomo
Differential Revision: 685
Summary: Basic integration between Phriction and feed.
Test Plan: Created and edited some documents, they published to feed.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 653
Summary:
This thing services every app but it lives inside Differential right now. Pull
it out, and separate the factory interfaces per-application.
This will let us accommodate changes we need to make for Phriction to support
wiki linking.
Test Plan: Tested remarkup in differential, diffusion, maniphest, people,
slowvote.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 646
Summary: see title
Test Plan: ran "arc amend" to ensure that task ids where being included in the commit message
Reviewers: epriestley
CC: dpepper
Differential Revision: 637
Summary: Basic hookup for Differential -> Feed. Also introduces "one-line"
stories for less-important stuff.
Test Plan: Interacted with some revisions, got feed stories out of it.
Reviewed By: jungejason
Reviewers: jungejason, aran, tuomaspelkonen, codeblock
CC: aran, jungejason
Differential Revision: 632
Summary:
Add the differential parse cache to the GC. This is the largest object in the
system by a wide margin, I think.
This table is potentially gigantic which is why the script truncates it before
doing a schema change.
Test Plan: Ran the GC daemon, it cleaned up some parse caches.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: tuomaspelkonen
CC: aran, jungejason, tuomaspelkonen, epriestley
Differential Revision: 620
Summary:
See T303. Enable comment panel haunting.
I hid the preview for the sticky panel, which I think is reasonable?
Test Plan:
https://secure.phabricator.com/file/view/PHID-FILE-64713fa8a7c2a22e5b93/
Reviewed By: broofa
Reviewers: broofa, jungejason, aran, tomo, tuomaspelkonen
CC: aran, broofa
Differential Revision: 615
Summary:
My earlier diff refactored some code without completely
respecting the semantics, sometimes resulting in duplicate field names
returned from differential.getcommitmessage. This fixes that.
Test Plan:
ran "arc diff" with diff causing the bug (commit message
had an empty Revert Plan: field) and verified no duplicate fields
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, dpepper, epriestley
Differential Revision: 610
Summary: Reduce the amount of code duplication here and allow for an override
configuration on the filename.map stuff.
Test Plan: Checked paste, diffusion and differential syntax highlighting and
everything appeared reasonable.
Reviewed By: codeblock
Reviewers: tuomaspelkonen, codeblock, jungejason, aran
CC: aran, codeblock, epriestley
Differential Revision: 601
Summary: I'll clean some of this stuff up in a followup too, but update the
callers to use the new explicit filename-based API.
Test Plan: Looked at paste, Diffusion and Differential.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, codeblock, jungejason, aran
CC: aran, tuomaspelkonen
Differential Revision: 600
Summary: These came up in dealing with the diff produced by T271. When a file is
unmodified, don't try to use the "ignore all whitespace" algorithm on it. Also,
detect "changed only by adding or removing trailing whitespace" vs "this file
was not modified" correctly.
Test Plan: Viewed the diff that came out of running 'arc diff' on my T271 mess.
Reviewed By: tuomaspelkonen
Reviewers: alex, jungejason, tuomaspelkonen, aran
CC: aran, tuomaspelkonen
Differential Revision: 548
Summary:
- Add a default list of supported languages to default.conf.php
and make the initial/default value customizable.
- Store a '' in the database to infer the language from the filename/title.
Test Plan:
Tested in my sandbox with pygments enabled and disabled and various
combinations of filename/extension/dropdown selection.
Reviewers:
epriestley
CC:
Differential Revision: 587
Summary:
We already support this (and Facebook uses it) but it is difficult to configure
and you have to write a bunch of code. Instead, provide a simple flag.
See the documentation changes for details, but when this flag is enabled we send
one email with a reply-to like "D2+public+23hf91fh19fh@phabricator.example.com".
Anyone can reply to this, and we figure out who they are based on their "From"
address instead of a unique hash. This is less secure, but a reasonable tradeoff
in many cases.
This also has the advantage over a naive implementation of at least doing object
hash validation.
@jungejason: I don't think this affects Facebook's implementation but this is an
area where we've had problems in the past, so watch out for it when you deploy.
Also note that you must set "metamta.public-replies" to true since Maniphest now
looks for that key specifically before going into public reply mode; it no
longer just tests for a public reply address being generateable (since it can
always generate one now).
Test Plan:
Swapped my local install in and out of public reply mode and commented on
objects. Got expected email behavior. Replied to public and private email
addresses.
Attacked public addresses by using them when the install was configured to
disallow them and by altering the hash and the from address. All this stuff was
rejected.
Reviewed By: jungejason
Reviewers: moskov, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, moskov, jungejason
Differential Revision: 563
Summary: This used to be in the subject but there was a bunch of churn and now
it's nowhere.
Test Plan: Created, updated, and added CCs to a diff.
Reviewed By: moskov
Reviewers: moskov, avitaloliver, jungejason, tuomaspelkonen, aran
CC: aran, moskov
Differential Revision: 567
Summary: The field hints on this interface don't behave correctly. Particularly, when you add yourself as a reviewer you aren't pointed at the issue.
Test Plan: Edited a revision and tried to save invalid changes, including self-reviewership.
Reviewers: moskov, jungejason, tuomaspelkonen, aran
CC:
Differential Revision: 565
explicitly
Summary: You currently have to click through to figure out who got added.
Test Plan:
- Made a comment which added CCs.
- Made a comment which added reviewers.
- Made a comment which added nothing.
- Made a comment which added CCs and reviewers.
Reviewed By: tomo
Reviewers: tomo, jungejason, tuomaspelkonen, aran
CC: aran, tomo
Differential Revision: 562
Summary:
when "arc diff" generates a revision, it attaches a task id
if one is included. However, "arc amend" did not return a task id,
effectively stripping it from the commit message. This diff fixes
that.
NOTE: This is dependent on revision 549 https://secure.phabricator.com/D549
Test Plan:
0. created a custom class to append Facebook task IDs to commit messages and
attached it to the differential.append-commit-message-class config variable
1. created a new diff in the www repot
2. included Task ID: 609350 in the git commit message
3. "arc diff" to generate the revision
4. "arc amend"
5. ensure that the "Task ID:" field remained in the git commit message
Reviewed By: epriestley
Reviewers: dpepper, jungejason, epriestley
CC: aran, epriestley, mgummelt
Differential Revision: 546
Summary:
commit message fields were previously stored as name/value
pairs in an associative array. this resulted in ad hoc code to modify
the structure/rendering of these fields in commit messages. this diff
introduces a new DifferentialCommitMessageField class.
Test Plan:
ran "arc amend" to ensure the commit message still looked good
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, dpepper, epriestley
Differential Revision: 554
Summary:
When a user gets @mentioned in Differential, add them as a CC.
No Maniphest hookup yet since I want to make that one a little more formal.
Depends on D518.
Test Plan:
@mentioned a user and they were added as a CC.
Reviewed By: jungejason
Reviewers: tomo, mroch, jsp, jungejason, aran, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 519
Summary:
We currently have only an "Add reviewers" action, add "Add CCs". This can also
be accomplished less-discoverably with mentions.
Test Plan:
Added reviewers and CCs to revisions. Toggled display between reviewers and CCs.
Reviewed By: jungejason
Reviewers: tomo, mroch, jsp, jungejason, aran, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 521
Summary:Make a new directory, src/infrastructure/markup/remarkup/markuprule/paste/
Make a new class called PhabricatorRemarkupRulePaste in that directory.
Add the rule to DifferentialMarkupEngineFactory.
Test Plan: Created a task in maniphest. Put P1 and P2 in the content.
Created P1 and P2 in Paste. Verified P1 and P2 were highlighted and
linked correctly.
Reviewers:epriestley, codeblock
CC:jungejason
Differential Revision: 539
Summary:
Although I think the recent changes here improved things, the "Unsaved Draft"
language is continuing to confuse new users. Try to find some less-confusing
langauge. Open to suggestions here, too.
Test Plan:
Viewed unsubmitted inline comments.
Reviewed By: jungejason
Reviewers: aran, jungejason, gregprice
CC: aran, epriestley, jungejason
Differential Revision: 501
Summary:
This depends on D513 and D514. Those diffs make the display algorithms safe with
respect to mutating utf8, so we no longer need to repair potentially invalid
utf8 sequences with this hack.
Test Plan:
grepped for calls to this method
Reviewers: jungejason, aran, tuomaspelkonen
Commenters: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 515
Summary:
Differential uses a byte-oriented linewrap algorithm. Instead, use a
character-oriented one which will handle utf-8 properly.
This implies a very slightly performance hit but we only run this code for lines
which need to wrap, and the results get cached. It took about ~2.5ms for the
test file on my machine. I'll keep an eye on it but I think it's currently a
manageable cost.
Test Plan:
Diffed this file: https://secure.phabricator.com/P43
...and got it to render like this:
https://secure.phabricator.com/file/info/PHID-FILE-331ac241bede705b193b/
To do so, I had to disable the un-utf8 block which we can't actually do yet
because of intraline diff, but it shows that once we can get rid of that it
works completely correctly. It will "sort of" work in the meantime (nothing
terrible happens).
Reviewers: jungejason, aran, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 513
Summary:
Replace some more date() calls with locale-aware calls.
Also, at least on my system, the DateTimeZone / DateTime stuff didn't actually
work and always rendered in UTC. Fixed that.
Test Plan:
Viewed daemon console, differential revisions, files, and maniphest timestamps
in multiple timezones.
Reviewed By: toulouse
Reviewers: toulouse, fratrik, jungejason, aran, tuomaspelkonen
CC: aran, toulouse
Differential Revision: 530
Summary:
Provides basic Remarkup support for @mentions. No application integration yet so
these aren't terribly useful until that happens.
Test Plan:
https://secure.phabricator.com/file/view/PHID-FILE-83d68e7af6085ae928df/
Reviewers: tomo, mroch, jsp
Commenters: tomo
CC: aran, tomo, epriestley
Differential Revision: 517
