Summary:
By default, PHP-FMP (an alternate PHP FCGI SAPI) cleans the entire environment
for child processes. This means we have no $PATH.
This causes some confusing failures for reasons I don't fully understand. If you
do these things:
exec_manual('env');
exec_manual('export');
...they show no $PATH, as expected. If you do this:
exec_manual('echo $PATH');
...it shows a path. And this works (i.e., it finds the executable):
exec_manual('ls');
...but this fails (it says "no ls in ((null))"):
exec_manual('which ls');
So, basically, the sh -c process itself gets a default PATH somehow, but its
children don't. I don't realllly get why this happens, but clearly an empty
$PATH is a misconfiguration, and can easily be remedied.
See discussion here: https://github.com/facebook/libphutil/issues/7
Test Plan: Applied patch to Centos6 + nginx + PHP-FPM machine, ran setup, the
configuration issue was detected and I was given information on resolving it.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1413
Summary:
A few people in IRC have been having issues here recently. If you misconfigure
the IRC bot, e.g., you get a 200 response back with a bunch of login HTML in it.
This is unhelpful.
Try to detect that a conduit request is going to the wrong path and raise a
concise, explicit error which is comprehensible from the CLI.
Also created a "PlainText" response and moved the IE nosniff header to the base
response object.
Test Plan: As a logged-out user, hit various nonsense with "?__conduit__=true"
in the URI. Got good error messages. Hit nonsense without it, got login screens.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T775
Differential Revision: https://secure.phabricator.com/D1407
Summary:
They are present in the document so there is not reason to omit the links to
them.
They sometimes contains changed lines so the link could be actualy useful.
Test Plan: Display ToC of revision with moved and copied files.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, nh
Differential Revision: https://secure.phabricator.com/D1412
Test Plan:
Open menu for added file
Open menu for deleted file
Open menu for changed file
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1410
Summary: D1354 added a query for a possibly-empty list -- only show the table if
there are transformations.
Test Plan: Reloaded a previously-fataling page, no fatals. Viewed a file with
transformations, got a list.
Reviewers: davidreuss, btrahan, jungejason
Reviewed By: davidreuss
CC: aran, davidreuss
Differential Revision: https://secure.phabricator.com/D1414
Summary: I've also moved the response generation for 404 from
##AphrontDefaultApplicationConfiguration## to ##buildResponseString()##
Test Plan:
Visit /
Visit /mail/
Visit /x/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, vrana
Differential Revision: https://secure.phabricator.com/D1406
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
Summary:
I always forget a branch which I used for the diff so that I must open
my browser which takes some time. This diff adds the name of the branch to the
sent e-mails. But only if the diff is in the state Accepted or Needs Revision to
not pollute other e-mails.
Test Plan:
Comment
Request changes
Accept
Look at the e-mails
Reviewers: epriestley
Reviewed By: epriestley
CC: olivier, aran, epriestley, vrana
Differential Revision: https://secure.phabricator.com/D1396
Test Plan:
Display diff with lint errors
Click on a line number in lint errors overview
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1400
Summary:
Commenting on a diff causes adding the writer to the CCs. It doesn't make much
sense if the writer is author or reviewer who get all the copies anyway.
I've also moved the decision to DifferentialCommentEditor.
Test Plan:
Comment on a diff where I am author
Comment on a diff where I am reviewer
Comment on a diff where I am neither
Explicitely Add CCs where I am author
Reviewers: epriestley
Reviewed By: epriestley
CC: jungejason, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1397
Summary:
I locked this down a little bit recently, but make
double-extra-super-sure that we aren't sending the user anywhere suspicious or
open-redirecty. This also locks down protocol-relative URIs (//evil.com/path)
although I don't think any browsers do bad stuff with them in this context, and
header injection URIs (although I don't think any of the modern PHP runtimes are
vulnerable).
Test Plan:
- Ran tests.
- Hit redirect page with valid and invalid next URIs; was punted to / for
invalid ones and to the right place for valid ones.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: arice, aran, epriestley, btrahan
Differential Revision: https://secure.phabricator.com/D1369
Summary:
- PHP uses a SAPI ("server API") to determine how it interacts with the caller
(e.g., how to read the environment, how to read flags, what code to execute).
- There are several different SAPIs: cli, cgi, cgi-fcgi, apache, etc.
- Each SAPI has different behavior -- for instance, the "cgi" SAPI emits some
CGI headers unless told not to, so a script like 'echo "x"' actually echoes some
headers and then 'x' as an HTTP body.
- In some setups, "php" may be php-cgi.
- If you run php-cgi as "php scriptname.php" and your ENV has an existing CGI
request in it, it runs that CGI request instead of the script. This causes an
infinite loop.
- Add checks to verify that "php" is the "cli" SAPI binary, not some other
SAPI.
- In particular, cPanel uses suphp and is affected by this configuration
issue. See this thread:
https://lists.marsching.com/pipermail/suphp/2008-September/002036.html
Test Plan:
- On a cPanel + suphp machine, ran setup and was stopped for having the
"cgi-fcgi" SAPI instead of throw into an infinite loop.
- Applied the suggested remedy, setup now runs fine.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1390
Summary: ..."ssh" is in quotes 'cuz this is step 1 and there's no ssh in sight
at the moment.
Test Plan:
ran api.php PHID-USER-xee4ju2teq7mflitwfcs differential.query a few times...
- tried valid input, it worked!
- tried bad input, it worked in that it failed and told me so!
ran api.php crap_user differential.query a few times...
- verified error message with respect to crap_user
ran api.php PHID-USER-xee4ju2teq7mflitwfcs crap_method a few times...
- verified error message with respect to crap_method
visited http://phabricator.dev/conduit/method/differential.query a few times...
- tried valid input, it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T550
Differential Revision: https://secure.phabricator.com/D1357
Summary: See T773 and the explanatory inline comment.
Test Plan: Made no-action comments and comments that did something (reject, plan
changes) to revisions. Saw them always jump to the top of the action list.
Reviewers: jungejason, simpkins, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T773
Differential Revision: https://secure.phabricator.com/D1386
Summary:
- Link to "importing a repository" from Config next steps, since it's not
obvious (and the article isn't obviously named).
- Some minor doc tweaks.
- Remove "Roadmap" document since it's super out of date and not very useful.
Test Plan: Regenerated and read documentation.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T743
Differential Revision: https://secure.phabricator.com/D1384
Summary:
- When a user is creating a Phriction document, save a draft as
"phriction:<slug>".
- When a user is editing a Phriction document, save a draft as "<document
phid>:<document version>".
- If a user has an available draft, use that instead of the native content.
- If using a draft, tell the user and give them an option to discard it.
- If a page is updated, your draft is lost (we show new page content
unconditionally) but this should be rare and is the simplest way to resolve this
issue in a realtively consistent way.
Test Plan:
- Recovered drafts for new and edited pages.
- Used "nodraft" to discard drafts.
Reviewers: davidreuss, btrahan, jungejason
Reviewed By: davidreuss
CC: aran, davidreuss
Maniphest Tasks: T769
Differential Revision: https://secure.phabricator.com/D1378
Summary:
engineers requested to supporting filtering by 'committed'
revisions, and I think it makes sense.
Test Plan: verified that all the three options worked
Reviewers: epriestley, btrahan, nh
Reviewed By: nh
CC: nh, wolffiex, aran
Differential Revision: https://secure.phabricator.com/D1383
Summary: If a remote address has too many recent login failures, require they
fill out a captcha before they can attempt to login.
Test Plan: Tried to login a bunch of times, then submitted the CAPTHCA form with
various combinations of valid/invalid passwords and valid/invalid captchas.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Maniphest Tasks: T765
Differential Revision: https://secure.phabricator.com/D1379
Summary:
- We currently run ##parseValueFromCommitMessage()## on all fields present in
the message, but not ##validateField()##.
- This detects value errors (e.g., an invalid reviewer) but not higher-level
errors (e.g., a missing field).
- This can break the stacked-commits Git mutable history workflow by
recognizing too many commit messages as valid ("multiple valid commit messages,
this is ambiguous").
- This also gives you some errors ("Missing test plan") too late in "arc diff
--create" (after the diff has been built).
Test Plan:
- Grepped for validateField() calls, removed a couple of calls that had the
same implementation as the base class.
- Grepped for other calls to this to make sure I'm not stumbling into
unintended side effects, but it only runs from the diff workflow.
- Ran "arc diff --create" with an invalid test plan, got a good error early in
the process.
- Ran "arc diff master" with stacked local commits, got a correct selection of
the intended message.
Reviewers: cpiro, btrahan, jungejason
Reviewed By: cpiro
CC: aran, cpiro
Differential Revision: https://secure.phabricator.com/D1373
interfaces
Summary:
- We have a hard-coded minimum length of 3 right now (and 1 in the other
interface), which is sort of silly.
- Provide a more reasonable default, and allow it to be configured.
- We have two password reset interfaces, one of which no longer actually
requires you to verify you own the account. This is more than a bit derp.
- Merge the interfaces into one, using either an email token or the account's
current password to let you change the password.
Test Plan:
- Reset password on an account.
- Changed password on an account.
- Created a new account, logged in, set the password.
- Tried to set a too-short password, got an error.
Reviewers: btrahan, jungejason, nh
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T766
Differential Revision: https://secure.phabricator.com/D1374
Summary:
Until T605 gets fixed, you might end up with a Project without a Profile if the
Profile insert failed. This fatals the list view; instead, don't fatal if a
profile is missing.
(At some point we should probably just merge this field into the Project object,
I was just mimicking the user/profile separation but we have partial-field
object support now and Projects aren't super heavily used or very big.)
Test Plan:
- Viewed list view including a project with a missing profile.
- Edited the project, creating its profile.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: arice, aran, btrahan
Differential Revision: https://secure.phabricator.com/D1368
Summary:
With T764, http://localhost doesn't work anymore. So add instructions
about how to support it by modifying the hosts file.
Test Plan:
- turned on setup mode and the error message did show up
- turned off the setup mode and the error message also showed up
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T764
Differential Revision: https://secure.phabricator.com/D1370
Summary:
- Add some captions to make it more clear what these fields mean.
- Require "name", since tokenizers use it exclusively.
- Limit URI to allowed protocols, since admins can currently XSS users by
entering a "javascript:" URI and then tricking the user into clicking the
mailing list name. This exploit is dumb, but technically privilege escallation.
Test Plan:
- Created a new mailing list.
- Edited a mailing list.
- Tested URI: valid, invalid, omitted.
- Tested name: valid, omitted.
Reviewers: btrahan, jungejason, davidreuss
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1365
Summary:
Added a Conduit API method to return all transactions for a
given set of task_ids. This will be used to comments and other important
information about the tasks.
Test Plan:
Use Conduit to execute ##maniphest.gettasktransactions## and
visually verify that transaction information is returned.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1361
Summary: @s reported an issue with implicit file:// URIs in Git, see P270.
Recognize and handle URIs in this format. For URIs we don't understand, raise an
exception.
Test Plan:
- Added failing tests.
- Fixed code.
- Tests pass.
Reviewers: btrahan, jungejason, s
Reviewed By: s
CC: aran, epriestley, s
Differential Revision: https://secure.phabricator.com/D1362
Summary:
- There are some recent reports of login issues, see T755 and T754. I'm not
really sure what's going on, but this is an attempt at getting some more
information.
- When we login a user by setting 'phusr' and 'phsid', send them to
/login/validate/ to validate that the cookies actually got set.
- Do email password resets in two steps: first, log the user in. Redirect them
through validate, then give them the option to reset their password.
- Don't CSRF logged-out users. It technically sort of works most of the time
right now, but is silly. If we need logged-out CSRF we should generate it in
some more reliable way.
Test Plan:
- Logged in with username/password.
- Logged in with OAuth.
- Logged in with email password reset.
- Sent bad values to /login/validate/, got appropriate errors.
- Reset password.
- Verified next_uri still works.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, j3kuntz
Maniphest Tasks: T754, T755
Differential Revision: https://secure.phabricator.com/D1353
Summary:
Chrome/Chromium won't set cookies on these domains, at least under
Ubuntu. See T754. Detect brokenness and explode.
Test Plan:
Logged into phabricator as "http://derps/" (failed) and
"http://derps.com/" (worked) in Chromium. Set config to "http://derps/" (config
exploded) and "http://local.aphront.com/" (config OK).
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T754
Differential Revision: https://secure.phabricator.com/D1355
Summary:
Just talked to @tuomaspelkonen, and turns out there is a case where
postponed tests results use the filepath for both the name and file
parameters. Then, after the tests have completed, the unittest
results are updated with the class name as the test name. To handle
this, this diff matches the stored unittest results name against
either the name or file component of the updated unittest info.
Not sure of great way to generally handle these situations. Perhaps,
long term, we can just use a placeholder unittest result, mark that
as passed (or delete it?) then add a new test result with the correct
name.
Test Plan: updated unittest result with new name (but file was the same).
Reviewers: epriestley, tuomaspelkonen
Reviewed By: epriestley
CC: aran, epriestley, andrewjcg
Differential Revision: https://secure.phabricator.com/D1356
Summary:
When using postponed unittests to make 'arc diff' faster, there
are some situations where it is difficult to know exactly how
many unittests will be run. This is the case for many of our
C++ unittests, which we can't really know until we compile the
tests (which is slow, and probably isn't reasonable to be done
before posting the diff). I suppose we could make sure we
explicitly which tests a C++ unittest will run in some way, but
this would require a lot of change to our backend test infra.
Also, it seems that this is a pretty general issue of not knowing
how many unittests will be run until they actually run.
This diff adds an optional "create" parameter to updateunitresults
which wil create a new unit tests result rather than updating an
existing one. I am not sure if this really fits here or should
be its own method, but there is a lot of code re-use between them
so I consolidated.
Test Plan: updated a diff with a new unit test result
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley, andrewjcg, tuomaspelkonen
Differential Revision: https://secure.phabricator.com/D1352
Summary:
we used to need this function for security purposes, but no longer need
it. remove it so that some call sites can be optimized via smarter data
fetching, and so the whole codebase can have one less thing in it.
Test Plan:
verified the images displayed properly for each of the following
- viewed a diff with added images.
- viewed a user feed
- viewed a user profile
- viewed all image macros
- viewed a paste and clicked through "raw link"
weakness in testing around proxy files and transformed files. not sure what
these are. changes here are very programmatic however.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T672
Differential Revision: https://secure.phabricator.com/D1354
Summary: When private messaged, the bot responds via private message to the
sender, instead of sending a private message to itself.
Test Plan: Mentioned tasks in public channels and private messages.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T274
Differential Revision: https://secure.phabricator.com/D1350
Phabricator
Summary: ...this breaks without D1328. Used good ole "codemod" to do this
work, with lots of manual edits around 80 chars.
Test Plan: clicked around phabricator tool suite, particular differential, a
bunch
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1351
Summary: We need some additional fields to heuristically match revisions to the
working copy in arc.
Test Plan: Executed conduit method, got correct values in fields
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1347
Summary: XHProf install documentation went missing a month or two ago (see T725)
and doesn't work in the widely deployed versions of PEAR/PECL. Provide
build-from-source instructions inline.
Test Plan: Generated, read documentation.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T725
Differential Revision: https://secure.phabricator.com/D1345
Summary: Simple notificaiton handler that reads the difx event timeline and
posts notifications to IRC.
Test Plan: Ran it in #phabricator.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1337
Summary:
The filename header for inline comments used to span 2 columns - the line number
and the comment. With the addition of a column for the diff (to link to inline
comments on previous diffs), the filename header should now span 3 columns
instead of just the line number and diff, leaving the comment squished to the
right.
Test Plan:
Opened a differential revision with an inline comment from a previous diff, and
saw that the filename header continued across the comment. Also checked an
inline comment on a current diff, and saw that it looks fine.
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1340
Summary: Not really thrilled about my fix for T684 in D1224. This makes some
design tweaks to solve it without the awkward horizontal scrollbar in the page
content div.
Test Plan: Looked at diffs overflowing the window. Looked at footer on several
pages.
Reviewers: btrahan, jungejason, Makinde
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T684
Differential Revision: https://secure.phabricator.com/D1332
Summary: Clicks all the "Show All" links for you at the touch of a button.
Test Plan:
- Used "reveal entire file" on revealable files.
- Opened on already-visible files, got "entire file shown".
- Used other menu options.
- Used normal "show more" links.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T497
Differential Revision: https://secure.phabricator.com/D1331
Summary:
We currently don't link to comments which aren't visible. Link to the
appropriate diff in a new window, indicating where the comment lives.
Test Plan: Clicked visible, not-so-visible comments.
Reviewers: btrahan, jungejason, davidreuss
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Maniphest Tasks: T555, T449
Differential Revision: https://secure.phabricator.com/D1333
Summary: Make it a little easier to create a bunch of accounts if your company
has more than like 5 employees.
Test Plan: Ran "add_user.php" to create new users. Created new users from the
web console.
Reviewers: btrahan, jungejason, rguerin
Reviewed By: btrahan
CC: aran, btrahan, rguerin
Differential Revision: https://secure.phabricator.com/D1336
Summary:
If you try to establish several sessions quickly (e.g., by running several
copies of "arc" at once, as in "arc x | arc y"), the current logic has a high
chance of making them all pick the same conduit session to refresh (since it's
the oldest one when each process selects the current sessions). This means they
all issue updates against "conduit-3" (or whatever) and one ends up with a bogus
session.
Instead, do an update against the table with the session key we read, so only
one process wins the race. If we don't win the race, try again until we do or
have tried every session slot.
Test Plan:
- Wiped conduit sessions, ran arc commands to verify the fresh session case.
- Ran a bunch of arc piped to itself, e.g. "arc list | arc list | arc list |
...". It succeeds up to the session limit, and above that gets failures as
expected.
- Manually checked the session table to make sure things seemed reasonable
there.
- Generally ran a bunch of arc commands.
- Logged out and logged in on the web interface.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T687
Differential Revision: https://secure.phabricator.com/D1329
Summary:
Provide an easy way to jump to Diffusion from Differential if we have
the data we need to connect them.
Test Plan: Tested menu in linked and unlinked diffs. Used menu item.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T309
Differential Revision: https://secure.phabricator.com/D1326
