1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-14 10:52:41 +01:00
Commit graph

10907 commits

Author SHA1 Message Date
Bob Trahan
871c06ab4e Calendar - error better with invalid time values
Summary: Fixes T7665. Unfortunately when the user messes this up its not easy to partially recover so we just reset that time to the default.

Test Plan: set time to "00:00 AM" and got a sensible error.

Reviewers: chad, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7665

Differential Revision: https://secure.phabricator.com/D12174
2015-03-26 13:02:49 -07:00
Chad Little
4ae28837fd Update Conpherence CSS to handle multiple edits better
Summary: Fixes T7655. We'll set tighter spacing around edit clusters. Also darkened up the date marker and remove unused `phabricator-transaction-view` CSS that was still scattered around the site.

Test Plan: Test a full and column multi-edit spam. Visited Ponder and Diffusion, noticed no issues using those apps. Grepped for other users of `phabricator-transaction-view`

Reviewers: epriestley, btrahan

Reviewed By: btrahan

Subscribers: Korvin, epriestley

Maniphest Tasks: T7655

Differential Revision: https://secure.phabricator.com/D12148
2015-03-26 12:56:58 -07:00
Bob Trahan
e4b7263bf8 Conpherence - Differentiate audience of Threads/Rooms with icon
Summary:
Fixes T7629 plus an un filed bug that's breaking creating new threads since we need to add participants EVEN EARLIER than we were doing it now that policy is actually enforced.

Back to the main thrust of this, there is one UI corner case - in the main view if you go from 1:1 to 1:1:1 (i.e. add a 3rd recipient, or Nth in a row) the icon only updates on page reload. I figure this will get sorted out at a later refactor as we make the client better / share more code with durable column.

One other small behavioral oddity is in the main view sometime we start loading with no conpherence. in that case, rather than show some incorrect icon, we show no icon (and "no title") and then things change at load. Seems okay-ish.

Finally, @chad - the CSS is a very work-man-like "use the built in stuff you can specify from PHP" so I'm sure it needs some love.

Test Plan: made all sorts of rooms and threads and liked the icons. noted smooth loading action as i switched around

Reviewers: chad, epriestley

Reviewed By: epriestley

Subscribers: Korvin, chad, epriestley

Maniphest Tasks: T7629

Differential Revision: https://secure.phabricator.com/D12163
2015-03-26 12:24:29 -07:00
epriestley
ffe654e5e3 Fix directory moves and copies in Subversion hosted repositories
Summary: Fixes T6490.

Test Plan:
```
$ svn mv dir/ dir2
A         dir2
D         dir
D         dir/list.txt

$ svn commit -m 'Move dir/ to dir2/'
Deleting       dir
Adding         dir2

Committed revision 3.

$ svn cp dir2/ dir3
A         dir3

$ svn commit -m 'Copy dir2/ to dir3/'
Adding         dir3

Committed revision 4.
```

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6490

Differential Revision: https://secure.phabricator.com/D12173
2015-03-26 11:13:41 -07:00
epriestley
a4bfed8415 Censor response bodies from Mercurial error messages
Summary:
Ref T6755. In Git and Subversion, running `git clone http://google.com/` or `svn checkout http://google.com/` does not echo the response body.

In Mercurial, it does. Censor it from the output of `hg pull` and `hg clone`. This prevents an attacker from:

  - Creating a Mercurial remote repository with URI `http://10.0.0.1/secrets/`; and
  - reading the secrets out of the error message after the clone fails.

Test Plan: Set a Mercurial remote URI to a non-Mercurial repository, ran `repository update`, saw censored error message.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6755

Differential Revision: https://secure.phabricator.com/D12170
2015-03-26 11:13:17 -07:00
epriestley
40fb0f98df Mostly defuse DNS rebinding attack for outbound requests
Summary: Ref T6755. I'll add some notes there about specifics.

Test Plan:
  - Made connections to HTTP and HTTPS URIs.
  - Added some debugging code to verify that HTTP URIs were pre-resolved.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6755

Differential Revision: https://secure.phabricator.com/D12169
2015-03-26 11:12:22 -07:00
epriestley
2e72e9ff31 Rate limit outbound requests in Macros
Summary:
Ref T6755. Although we do not return response bodies, it is possible to perform crude portscanning if you can execute a DNS rebinding attack (which, for now, remains theoretical).

Limit users to 60 requests / hour to make it less feasible. This would require ~30 years to portscan all ports on a `/32` netblock.

Users who can guess that services may exist can confirm their existence more quickly than this, but if the attacker already had a very small set of candidate services it seems unlikely that portscanning would be of much use in executing the attack.

This protection should eventually be applied to T4190, too (that task also has other considerations).

Test Plan: Set rate limit very low, hit rate limit.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6755

Differential Revision: https://secure.phabricator.com/D12168
2015-03-26 11:11:52 -07:00
epriestley
cce6d06fa5 Move abandoned revisions to "needs review" when updated
Summary:
Fixes T7602. This is similar to the existing behavior for "changes planned" and "needs revision" revisions.

Also fix the "Update Diff" workflow so it correctly selects closed revisions as attachable.

Test Plan: Updated an abandoned revision, saw it change to "Needs Review".

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7602

Differential Revision: https://secure.phabricator.com/D12167
2015-03-26 11:11:33 -07:00
epriestley
731404445f Improve task subpriority movement algorithm for homogenous blocks
Summary:
Fixes T7664. When there are a large number of tasks (400+) with the same subpriority (which can happen if the subpriority features are rarely used), it may take more than 30 seconds to rebalance them.

Make the algorithm more aggressive about rebalancing homogenous blocks of tasks.

This may need to get even fancier, but I'd guess it can process blocks 1-2 orders of magnitude larger, which should be ~all installs.

(If someone still hits issues with this, I'll make it fancier.)

Once a block is rebalanced, it doesn't need to be rebalanced again (at least, not as a whole block) so we basically just need to get over the initial hurdle here and then we're good.

In the worst case, we can provide `bin/maniphest rebalance` or similar and do the rebalance step offline.

And, in any case, we have more test coverage here now.

Test Plan:
  - Existing tests.
  - New tests.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7664

Differential Revision: https://secure.phabricator.com/D12166
2015-03-26 11:11:23 -07:00
Chad Little
4bdc51237a Add ability to have tooltips on buttons
Summary: Enables a basic tooltip when using icon buttons and a convenience method for setting an icon.

Test Plan: Built a UIExample.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12172
2015-03-26 11:09:20 -07:00
Elan Kugelmass
fe89d67663 Fixes spelling error in settings log on auth provider pages
Summary: The settings logs on auth provider pages shows "enabled accont linking" instead of "enabled account linking."

Test Plan: Checked the copy on the settings log.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12164
2015-03-26 03:49:58 -07:00
Chad Little
47114513b0 More SIMPLE button styles for buttons and button bars
Summary: Ref T1460, this adds additional buttons colors and styles for use in inline comments. Will also backport to Calendar and PHUIInfoView

Test Plan:
Review new buttons and hover states in UI Examples.

{F350549}

{F350550}

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T1460

Differential Revision: https://secure.phabricator.com/D12162
2015-03-25 12:51:54 -07:00
Bob Trahan
42a0229a52 Conpherence - Implement edit rules for rooms
Summary: Fixes T7586. If you can't edit a room, the pertinent UI is greyed out. One exception is the title of the room in the full viewer; this crumb is not disabled as it would be hard to read. Otherwise though, everything is disabled nicely.

Test Plan: tried to add participants when I wasn't allowed to and got an error. added participants otherwise okay. tried to edit title when i wasn't allowed and got an error. otherwise okay. left conpherence threads / rooms successfully.

Reviewers: epriestley, chad

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7586

Differential Revision: https://secure.phabricator.com/D12161
2015-03-25 11:48:22 -07:00
epriestley
1fd163d097 Mostly provide CSS for "done" states
Summary: Ref T7660. I'm not toggling "inline-state-is-draft" correctly in JS yet since it's a little tricky (you can reload to see it) but the main state should work.

Test Plan:
  - Clicked "done", saw comment opacity fade with placeholder style.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T7660

Differential Revision: https://secure.phabricator.com/D12160
2015-03-25 10:57:08 -07:00
epriestley
e5445de163 Show only recent open revisions affecting the same files
Summary: Fixes T5658. Over a long period of time, some cruft can build up here. Only show revisions which have been updated in the last 30 days.

Test Plan:
  - Viewed panel in Differential and Diffusion.
  - Changed limit from 30 days to 30 seconds and saw no revisions.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5658

Differential Revision: https://secure.phabricator.com/D12158
2015-03-25 10:21:56 -07:00
epriestley
6ce4044bfa Lock MIME type configuration
Summary:
Ref T6755. This mitigates an attack where you:

  - compromise an administrative account;
  - configure "text/plain" as an "image" MIME type; and
  - create a new macro sourced from a sensitive resource which is locally accessible over HTTP GET, using DNS rebinding.

You can then view the content of the resource in Files. By preventing the compromised account from reconfiguring the MIME types, the server will instead destroy the response and prevent the attacker from seeing it.

In general, these options should change very rarely, and they often sit just beyond the edge of security vulnerabilities anyway.

For example, if you ignore the warnings about an alternate file domain and elect to serve content from the primary domain, it's still somewhat difficult for an attacker to exploit the vulnerability. If they can add "text/html" or "image/svg+xml" as image MIME types, it becomes trivial. In this case not having an alternate domain is the main issue, but easy modification of this config increases risk/exposure.

Test Plan: Viewed affected config and saw that it is locked.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6755

Differential Revision: https://secure.phabricator.com/D12154
2015-03-25 10:16:22 -07:00
epriestley
17e1e7a65a Document the need to purge caches after updating differential.generated-paths
Summary: Fixes T6378.

Test Plan: Set config to `/.*/`, created a new diff, everything was collapsed as generated.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T6378

Differential Revision: https://secure.phabricator.com/D12159
2015-03-25 07:29:09 -07:00
epriestley
c03297ab5a Fix interaction between undo and inline comment placement
Summary:
Fixes T7658. Currently, we remove the "undo" before placing the comment, but that causes us to lose track of which row we should be examining.

Instead, place the comment first, then remove the "undo".

Test Plan: This stuff is hard to test comprehensively, but the original report reproduced easily and is now fixed. I wasn't able to break anything by adding/editing/deleting comments.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T7658

Differential Revision: https://secure.phabricator.com/D12157
2015-03-25 07:14:12 -07:00
epriestley
4f8147dbb8 Improve protection against SSRF attacks
Summary:
Ref T6755. This improves our resistance to SSRF attacks:

  - Follow redirects manually and verify each component of the redirect chain.
  - Handle authentication provider profile picture fetches more strictly.

Test Plan:
  - Tried to download macros from various URIs which issued redirects, etc.
  - Downloaded an actual macro.
  - Went through external account workflow.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6755

Differential Revision: https://secure.phabricator.com/D12151
2015-03-24 18:49:01 -07:00
epriestley
22b2b8eb89 Fix a bad call in file chunk destruction
Summary: This signature changed at some point after I tested things and I didn't catch it.

Test Plan: Destroyed a chunked large file with `bin/remove`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D12152
2015-03-24 18:48:51 -07:00
Bob Trahan
25767096c9 Conpherence - implement join / view rules for rooms
Summary:
Ref T7585. This implements everything specified, with a few caveats

- since rooms you have yet to join can't be viewed in the column yet, the column view has some bugs and isn't expected to work.
- the room you're looking at is just pre-pending to the top of the "recent" list

Test Plan: made a room that no one could join. verified when viewing that there was no comment ui. made a room that others could join. verified folks who had yet to join had a "join" button with an area for text. tried joining with / without message text and it worked in both cases

Reviewers: chad, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7585

Differential Revision: https://secure.phabricator.com/D12149
2015-03-24 18:38:16 -07:00
epriestley
aa310230b6 Detect moves and copies with some unchanged lines as moves or copies
Summary:
Ref T1266. We won't detect a move/copy if fewer than 3 lines are changed.

However, you may move a block like:

  Complicated Line A
  Trivial Line B
  Complicated Line C

...where "Trivial Line B" is something like a curly brace. If you move this block somewhere that happened to previously have a similar trivial curly brace line, we won't be able to find 3 contiguous added lines in order to detect the copy/move.

Instead, consider both changed and unchanged lines when trying to find contiguous blocks. This allows us to detect across gaps where lines were not actually changed.

This new algorithm may be too liberal (for example, we may end up incorrectly identifying moved/copied code before or after changed lines, not just between changed lines), but we can keep an eye on it and tweak it. The algorithm is better factored and better covered, now.

Test Plan:
  - Added a unit test for this case.
  - Spot-checked a handful of diffs and generally saw behavior that made sense and looked better than before.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1266

Differential Revision: https://secure.phabricator.com/D12146
2015-03-24 13:12:24 -07:00
epriestley
373aaa643a Clean up copy detection code a bit
Summary:
Ref T1266. This doesn't change any behaviors, but some of this code has a lot of really complicated conditionals and I tried to break that up a bit.

Also, reexpress this stuff in terms of the "structured" parser in D12144.

Test Plan: Unit tests still pass. They aren't hugely comprehensive but did reliably fail when I screwed stuff up.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1266

Differential Revision: https://secure.phabricator.com/D12145
2015-03-24 13:12:09 -07:00
epriestley
74a4c2cf0b Provide better parsing primitives for hunks
Summary:
Ref T1266. This prepares to fix case (2) on T1266 by improving the robustness of hunk parsing.

In particular, the copy detection code abuses this API because it isn't currently expressive or flexible enough.

Make it more flexible and cover it exhaustively.

I'll move callsites to the new stuff in upcoming revisions.

Test Plan: Unit tests.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1266

Differential Revision: https://secure.phabricator.com/D12144
2015-03-24 13:11:37 -07:00
Bob Trahan
dcaafd6159 Conpherence - grey out username mentions if they aren't in the conpherence
Summary: Fixes T7578. This was pretty easy because conpherence funnels all transacton stuff through this spot

Test Plan: made a new room so only my user was a participant. wrote "@myself will work and @anotherguy will be greyed out" and so it was as expected

Reviewers: chad, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7578

Differential Revision: https://secure.phabricator.com/D12114
2015-03-24 13:08:53 -07:00
Bob Trahan
014bb72050 Conpherence - add "room" search UI and create UI
Summary: Ref T7584. This hits all the major bullets there. Next step on T7584 is figuring out how it integrates into the full UI and column UI. That said, this is a bit buggy feeling right now since Conpherence as is assumes you are a participant all over the place and rooms make no such assumption. I'll probably this bit up next.

Test Plan:
viewed /conpherence/room/ and saw stuff. viewed the "participant" query as two different users and saw different correct result sets. made a room via the button and it worked. tried to view a room I wasn't a participant in and it failed horribly, which is something to fix in a future diff

created a thread via "send message" on a user profile and it worked

Reviewers: chad, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7584

Differential Revision: https://secure.phabricator.com/D12113
2015-03-24 13:04:33 -07:00
epriestley
86404a1a18 Fix handling of notifications with project members
Summary: Fixes T7377. We don't expand projects into members when sending notifications right now. Instead, expand them.

Test Plan:
  - Added a project as a reviewer to a revision, made a comment, saw project members receive a read notification + email (with appropriate preferences).
  - There's meaningful test coverage on the core mail stuff.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7377

Differential Revision: https://secure.phabricator.com/D12142
2015-03-24 12:47:38 -07:00
epriestley
0efae2858e Don't syntax highlight codebase pattern search results
Summary:
Ref T5644. Ref T7472. Currently, we highlight each line of pattern search results in Diffusion.

  - This is incredibly slow for non-PHP languages which need to shell out to Pygments.
  - A lot of this highlighting isn't very useful anyway, because it doesn't have any context.

Instead, try to highlight pattern matches but don't highlight the source itself.

Test Plan: {F349637}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7472, T5644

Differential Revision: https://secure.phabricator.com/D12141
2015-03-24 12:47:28 -07:00
epriestley
21826ed7b3 Don't highlight very large files by default
Summary:
Ref T5644. See some discussion in D8040.

When a file is very large (more than 64KB of text), don't activate syntax highlighting by default. This should prevent us from wasting resources running `pygmentize` on enormous files.

Users who want the file highlighted can still select "Highlight As...".

The tricky part of this diff is separating the headers into "changeset" headers and "undershield" (rendering) headers. Specifically, a file might have these headers/shields:

  - "This file is newly added."
  - "This file is generated. Show Changes"
  - "Highlighting is disabled for this large file."

In this case, I want the user to see "added" and "generated" when they load the page, and only see "highlighting disabled" after they click "Show Changes". So there are several categories:

  - "Changeset" headers, which discuss the changeset as a whole (binary file, image file, moved, added, deleted, etc.)
  - "Property" headers, which describe metadata changes (not relevant here).
  - "Shields", which hide files from view by default.
  - "Undershield" headers, which provide rendering information that is only relevant if there is no shield on the file.

Test Plan:
  - Viewed a diff with the library map, clicked "show changes", got a "highlighting disabled" header back with highlighting disabled.
  - Enabled highlighting explicitly (this currently restores the shield, which it probably shouldn't, but that feels out of scope for this change). The deshielded file is highlighted per the user's request.
  - Loaded context on normal files.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T5644

Differential Revision: https://secure.phabricator.com/D12132
2015-03-24 05:26:39 -07:00
epriestley
dd3afe2aa2 Lift inline comment state transactions into core (in Differential)
Summary: Ref T1460. Follows D12129 and reduces code duplication.

Test Plan: Changed inline state in Differential.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1460

Differential Revision: https://secure.phabricator.com/D12130
2015-03-24 05:26:16 -07:00
epriestley
8c053f02a7 Lift inline state transactions into core (in Diffusion)
Summary:
Ref T1460. Ref T6403. Replace `Diffusion::INLINEDONE` with `Transactions::INLINESTATE` and generalize things enough that we can lift it into core.

The next change will lift Differential's similar implementation into the core.

Also start implementing a fix for T6403, providing an alternate hook for optional builtin transactions.

Test Plan: Changed inline state in Diffusion.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6403, T1460

Differential Revision: https://secure.phabricator.com/D12129
2015-03-24 05:26:14 -07:00
epriestley
cbb5a297d5 Publish "done" inline comment checkbox state in Diffusion
Summary:
Ref T1460. See D12126. This is essentially the same change, but for Diffusion.

This is a bit copy/pastey. I'm going to make an effort to lift inline handling into the core before pushing this in, so hopefully that will clean things up a bit.

Test Plan: Submitted stuff in Diffusion and got checkmarks to publish.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1460

Differential Revision: https://secure.phabricator.com/D12128
2015-03-24 05:26:13 -07:00
epriestley
9f3210c883 Publish draft "done" status when submitting comments/updates/actions/inlines
Summary:
Ref T1460. When a revision author updates/comments/etc on a revision, publish all their checkmarks.

This doesn't handle Diffusion/audits yet.

Test Plan: {F346870}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: yelirekim, epriestley

Maniphest Tasks: T1460

Differential Revision: https://secure.phabricator.com/D12126
2015-03-24 05:26:12 -07:00
epriestley
4310c4ed53 Track a "Done" state on inline comments
Summary:
Ref T1460. This just barely works, but throwing it up in case any of it sounds mechanically crazy before we build integrations/UI/etc.

Specifically, these are the behaviors:

  - You can mark your own draft comments as "done" before you submit them. The intent is to let reviewers mark their stuff advisory/minor/not-important before they submit it, to hint to authors that they don't expect the feedback to necessarily be addressed (maybe it's a joke, maybe it's just discussion, maybe it's "consider..").
  - You can mark others' published comments as "done" if you're the revision/commit author. The intent is to keep this lightweight by not requiring an audit trail of who marked what done when. If anyone could mark anything done, we'd have to have some way to show who marked stuff.
  - When you mark stuff done (or unmark it), it goes into a "draft" state, where you see the change but others don't see it yet. The intent is twofold:
    - Be consistent with how inlines work.
    - Allow us to publish a "epriestley updated this revision + epriestley marked 15 inlines as done" story later if we want. This seems more useful than publishing 15 "epriestley marked one thing as done" stories.
  - The actual bit where done-ness publishes isn't implemented.
  - UI is bare bones.
  - No integration with the rest of the UI yet.

Test Plan: Clicked some checkboxes.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: paulshen, chasemp, epriestley

Maniphest Tasks: T1460

Differential Revision: https://secure.phabricator.com/D12033
2015-03-24 05:26:11 -07:00
epriestley
d0b3f199bb Provide a smoother "update diff" web workflow
Summary:
Fixes T1102. If you don't use `arc`, the web workflow requires some extra needless steps when updating diffs.

Provide a more streamlined "Update Diff" workflow.

Test Plan: {F347750}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1102

Differential Revision: https://secure.phabricator.com/D12131
2015-03-23 10:44:33 -07:00
epriestley
1c32c9b965 Improve granluarity and defaults of security.allow-outbound-http
Summary:
Ref T6755. This is a partial fix, but:

  - Allow netblocks to be blacklisted instead of making the feature all-or-nothing.
  - Default to disallow requests to all reserved private/local/special IP blocks. This should generally be a "safe" setting.
  - Explain the risks better.
  - Improve the errors rasied by Macro when failing.
  - Removed `security.allow-outbound-http`, as it is superseded by this setting and is somewhat misleading.
    - We still make outbound HTTP requests to OAuth.
    - We still make outbound HTTP requests for repositories.

From a technical perspective:

  - Separate URIs that are safe to link to or redirect to (basically, not "javascript://") from URIs that are safe to fetch (nothing in a private block).
  - Add the default blacklist.
  - Be more careful with response data in Macro fetching, and don't let the user see it if it isn't ultimately valid.

Additionally:

  - I want to do this check before pulling repositories, but that's enough of a mess that it should go in a separate diff.
  - The future implementation of T4190 needs to perform the fetch check.

Test Plan:
  - Fetched a valid macro.
  - Fetched a non-image, verified it didn't result in a viewable file.
  - Fetched a private-ip-space image, got an error.
  - Fetched a 404, got a useful-enough error without additional revealing response content (which is usually HTML anyway and not useful).
  - Fetched a bad protocol, got an error.
  - Linked to a local resource, a phriction page, a valid remote site, all worked.
  - Linked to private IP space, which worked fine (we want to let you link and redierect to other private services, just not fetch them).
  - Added and executed unit tests.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6755

Differential Revision: https://secure.phabricator.com/D12136
2015-03-23 10:44:03 -07:00
Chad Little
5f7b7f24d6 Hide footer on mobile app switcher
Summary: Fixes T7609. When we moved the sidenav background to z-index -1, the footer then reappeared. Make the hiding more specific in CSS.

Test Plan: test mobile layout of homepage, check footer still exists on all other pages.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7609

Differential Revision: https://secure.phabricator.com/D12138
2015-03-23 09:56:39 -07:00
epriestley
c7178b7e7b Move property transaction construction to Almanac
Summary: Ref T7627. This centralizes this transaction construction code so the unit tests and Instances can both use it.

Test Plan: See D12116.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7627

Differential Revision: https://secure.phabricator.com/D12118
2015-03-23 09:10:42 -07:00
epriestley
6eadfe6a6f Allow repositories to be ordered by commit count
Summary: Fixes T7640.

Test Plan: {F346553}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7640

Differential Revision: https://secure.phabricator.com/D12122
2015-03-23 09:10:34 -07:00
epriestley
ae03733378 Fix matching of very short project hashtags ending in a digit
Summary: Fixes T7625. The way the regexp worked, "unusual" terminal characters required at least one character as a prefix in order to match. Allow 0 instead, so `#a1` matches.

Test Plan: Added and executed unit test.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7625

Differential Revision: https://secure.phabricator.com/D12123
2015-03-23 09:10:26 -07:00
epriestley
5001aadf46 Throw a more helpful error for bad Differential actions
Summary: Ref T7611. This should let us figure out the root cause, hopefully.

Test Plan: iiam

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7611

Differential Revision: https://secure.phabricator.com/D12124
2015-03-23 09:10:18 -07:00
epriestley
08aefafef7 Remove redundant administrator requirement from application edit policy page
Summary:
Fixes T7485. Before applications had proper policies, we gated access by requiring the viewer be an administrator.

This is now redundant (CAN_EDIT on applications has the same effect, and performs the same check), and may some day be wrong (we might let administrators configure a different policy to control who can configure applications). Today, it gets the policy dialog wrong.

Test Plan:
Clicked "Edit Policies" as a non-administrator, was unable to, got nice error:

{F346598}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7485

Differential Revision: https://secure.phabricator.com/D12125
2015-03-23 09:10:10 -07:00
epriestley
c7dc59f9c4 Don't call flush() when emitting responses
Summary: Fixes T7620. I don't fully understand exactly what's going on here, but we don't actually need to call `flush()`.

Test Plan:
  - Put timing code around the `echo`.
  - Made a fake page that emitted a lot of data.
  - Saw the `echo` block proportionate to data size under `curl --limit-rate ...`.
  - See T7620.
  - Downloaded a large file, got a reasonable progress bar and no obvious memory use issues.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: jlarouche, rbalik, epriestley

Maniphest Tasks: T7620

Differential Revision: https://secure.phabricator.com/D12127
2015-03-23 09:09:45 -07:00
Chad Little
c4d2fb087e Clear floats around conpherence-edited transactions
Summary: Fixes T7647. We float both the time and description here and want the fluidity.

Test Plan: Review a new Conpherence in FF, Safari, Chrome.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7647

Differential Revision: https://secure.phabricator.com/D12137
2015-03-23 09:09:04 -07:00
Povilas Balzaravicius Pawka
b4d0de6b96 T7646: Fix buildplan ac on Herald.
Summary: Fixes T7646.

Test Plan: Repeat steps described in T7646 and expect disabled build plans not displayed.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T7646

Differential Revision: https://secure.phabricator.com/D12133
2015-03-23 06:19:17 -07:00
epriestley
5aca529980 Fix literally thousands of drag-to-reorder priority bugs
Summary:
Fixes T7563. Fixes T5201. Reframe this as two separate operations:

  - Move before or after a task.
  - Move to the beginning or end of a priority.

Then:

  - Make all the order queries unambiguous and properly reversible, with an explicit `id` order.
  - Just reuse `ManiphestTask` to get results in the correct order.
  - Simplify the actual transaction apply logic.
  - Detect and recover from cases where tasks have identical or similar subpriorities.

Test Plan:
  - Wrote and executed unit tests.
  - Dragged and dropped tasks within priorities and between priorities in the main Maniphest view.
  - Dragged and dropped tasks within priorities in the workboard view, when ordered by priority.
  - Also poked at the "natural" order, but that shouldn't be affected.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: chad, epriestley

Maniphest Tasks: T5201, T7563

Differential Revision: https://secure.phabricator.com/D12121
2015-03-20 17:38:25 -07:00
epriestley
ac029d0a50 Fix a self-XSS hole in Diffusion
Summary:
Via HackerOne. We aren't correctly escaping the date, so a user can XSS themselves by setting their date format creatively.

This construction is very unusual and I don't think we do anything similar elsewhere, so I can't come up with a systematic change which would prevent this in the general case.

Test Plan: Set date format to tag junk, got self-XSS before patch and proper escaping after the patch.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D12117
2015-03-20 14:54:35 -07:00
epriestley
80b8dc521d Fix Mercurial command injection vulnerability
Summary: See <http://chargen.matasano.com/chargen/2015/3/17/this-new-vulnerability-mercurial-command-injection-cve-2014-9462.html>.

Test Plan: Crafted bad remote URL; got error instead of code execution.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D12112
2015-03-20 09:26:32 -07:00
epriestley
b7fa55ff93 Fix improper selection of the chunk engine as a writable engine
Summary:
Fixes T7621. The engine selection code started out making sense, but didn't make as much sense by the time I was done with it.

Specifically, from the vanilla file upload, we may incorrectly try to write directly to the chunk storage engine. This is incorrect, and produces a confusing/bad error.

Make chunk storage engines explicit and don't try to do single-file one-shot writes to them.

Test Plan:
  - Tried to upload a large file with vanilla uploader, got better error message.
  - Uploaded small and large files with drag and drop.
  - Viewed {nav Files > Help/Options}.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7621

Differential Revision: https://secure.phabricator.com/D12110
2015-03-18 19:06:39 -07:00
epriestley
bd2eaad04f Add "phabricator.silent" for stopping all outbound events from an install
Summary:
Ref T7522. This is mostly useful in the cluster, but could be useful for external installs too.

If you want to import an instance into a test/dry-run state (in the cluster, to test an import; in the general case, to do something like test new hardware or configuration), you currently risk spamming users with a lot of duplicate notifications. In particular, if Phabricator tracks remotes, both instances will continue importing commits and sending email about them. Both instances will try to publish to mirrors, too, which could be bad news, and both instances will try to update linked services.

Instead, provide a flag to let an instance run in "silent mode", which disables all outbound messaging and data.

We need to remember to support this flag on any new outbound channels, but we add about one of those per year so I think that's reasonable.

Test Plan:
  - Flipped config.
  - Saw it void email, feed and mirroring.
  - Didn't test SMS since it's not really in use yet and not convenient to test.
  - (Can you think of any publishing I missed?)

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7522

Differential Revision: https://secure.phabricator.com/D12109
2015-03-18 07:09:43 -07:00