Summary:
Ref T11140. This makes encryption actually work:
- Provide a new configuation option, `keyring`, for specifying encryption keys.
- One key may be marked as `default`. This activates AES256 encryption for Files.
- Add `bin/files generate-key`. This is helps when generating valid encryption keys.
- Add `bin/files encode`. This changes the storage encoding of a file, and helps test encodings and migrate existing data.
- Add `bin/files cycle`. This re-encodes the block key with a new master key, if your master key leaks or you're just paraonid.
- Document all these options and behaviors.
Test Plan:
- Configured a bad `keyring`, hit a bunch of different errors.
- Used `bin/files generate-key` to try to generate bad keys, got appropriate errors ("raw doesn't support keys", etc).
- Used `bin/files generate-key` to generate an AES256 key.
- Put the new AES256 key into the `keyring`, without `default`.
- Uploaded a new file, verified it still uploaded as raw data (no `default` key yet).
- Used `bin/files encode` to change a file to ROT13 and back to raw. Verified old data got deleted and new data got stored properly.
- Used `bin/files encode --key ...` to explicitly convert a file to AES256 with my non-default key.
- Forced a re-encode of an AES256 file, verified the old data was deleted and a new key and IV were generated.
- Used `bin/files cycle` to try to cycle raw/rot13 files, got errors.
- Used `bin/files cycle` to cycle AES256 files. Verified metadata changed but file data did not. Verified file data was still decryptable with metadata.
- Ran `bin/files cycle --all`.
- Ran `encode` and `cycle` on chunked files, saw commands fail properly. These commands operate on the underlying data blocks, not the chunk metadata.
- Set key to `default`, uploaded a file, saw it stored as AES256.
- Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11140
Differential Revision: https://secure.phabricator.com/D16127
Summary:
Ref T11140. This doesn't do anything yet since there's no way to enable it and no way to store master keys.
Those are slightly tougher problems and I'm not totally satisfied that I have an approach I really like for either problem, so I may wait for a bit before tackling them. Once they're solved, this does the mechanical encrypt/decrypt stuff, though.
This design is substantially similar to the AWS S3 server-side encryption design, and intended as an analog for it. The decisions AWS has made in design generally seem reasonable to me.
Each block of file data is encrypted with a unique key and a unique IV, and then that key and IV are encrypted with the master key (and a distinct, unique IV). This is better than just encrypting with the master key directly because:
- You can rotate the master key later and only need to re-encrypt a small amount of key data (about 48 bytes per file chunk), instead of re-encrypting all of the actual file data (up to 4MB per file chunk).
- Instead of putting the master key on every server, you can put it on some dedicated keyserver which accepts encrypted keys, decrypts them, and returns plaintext keys, and can send it 32-byte keys for decryption instead of 4MB blocks of file data.
- You have to compromise the master key, the database, AND the file store to get the file data. This is probably not much of a barrier realistically, but it does make attacks very slightly harder.
The "KeyRing" thing may change once I figure out how I want users to store master keys, but it was the simplest approach to get the unit tests working.
Test Plan:
- Ran unit tests.
- Dumped raw data, saw encrypted blob.
- No way to actually use this in the real application yet so it can't be tested too extensively.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11140
Differential Revision: https://secure.phabricator.com/D16124
Summary: Fixes T11156. These were never correct, but also never actually used until I made timelines load object handles unconditionally in D16111.
Test Plan: Viewed an auth provider with transactions, no more fatal.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11156
Differential Revision: https://secure.phabricator.com/D16128
Summary: Uses PHUITwoColumnView in Blog Manage and Blog Picture. Ref T9897
Test Plan: Use each page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16126
Summary: Adds some ordering options to PhamePost queries. Works on search, PhameHome, BlogHome
Test Plan: Try searching with Order By set to Date Published in application search, get correct order. Check a blog home page, check PhameHome.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16125
Summary:
Ref T11140. When reading and writing files, we optionally apply a "storage format" to them.
The default format is "raw", which means we just store the raw data.
This change modularizes formats and adds a "rot13" format, which proves formatting works and is testable. In the future, I'll add real encryption formats.
Test Plan:
- Added unit tests.
- Viewed files in web UI.
- Changed a file's format to rot13, saw the data get rotated on display.
- Set default format to rot13:
- Uploaded a small file, verified data was stored as rot13.
- Uploaded a large file, verified metadata was stored as "raw" (just a type, no actual data) and blob data was stored as rot13.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11140
Differential Revision: https://secure.phabricator.com/D16122
Summary: Ref T11142. H264 video in a Quicktime container works in Safari and Firefox for me (although not Chrome), so include it in the default video mime types.
Test Plan: Uploaded video file from T11142 locally, saw it render with `<video />` properly in Safari.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11142
Differential Revision: https://secure.phabricator.com/D16121
Summary: Flips the bits from true to false in transaction editor.
Test Plan: update a post, search for new term
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16120
Summary: Ref T9897, makes blogs searchable
Test Plan: Make a blog, index it, search for it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16119
Summary:
Ref T11098. Mixture of issues here:
- Similar problem to D16112, where users with no settings at all could fail to fall back to the global defaults.
- I made `UserPreferencesQuery` responsible for building defaults instead to simplify this, since we have 4 or 5 callsites which need to do it and they aren't easily reducible.
- Handle cases where `metamta.one-mail-per-recipient` is off (and thus users can not have any custom settings) more explicitly.
- When `metamta.one-mail-per-recipient` is off, remove the "Email Format" panel for users only -- administrators can still access it in global preferences.
Test Plan:
- Deleted a user's preferences, changed globals, purged cache, made sure defaults reflected global defaults.
- Changed global mail tags, sent mail to the user, verified it was dropped in accordinace with global settings.
- Changed user's settings to get the mail instead, verified mail was sent.
- Toggled user's Re / Vary settings, verified mail subject lines reflected user settings.
- Disabled `metamta.one-mail-per-recipient`, verified user "Email Format" panel vanished.
- Edited "Email Format" in single-mail-mode in global prefs as an administrator.
- Sent more mail, verified mail respected new global settings.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16118
Summary: Ref T11098. There is no reason to maintain these as separate values now that they can be configured in global settings.
Test Plan:
- Hit and read setup issue.
- Fiddled with settings.
- I'll vet this more throughly in the next diff since I need to fix an issue with global defaults in mail and can explicitly test this at the same time.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16117
Summary: Adds PhamePost object to fulltextsearch index. Some issue searching just "Open" though? Also "closed" objects search fine but don't display as disabled.
Test Plan:
bin/search index --type POST
{F1687043}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16116
Summary: Ref T9789. Falling back to `parent::` is better, and fixes older-style feed stories for Pastes, like "added a comment".
Test Plan: Viewed a comment feed story about a paste.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9789
Differential Revision: https://secure.phabricator.com/D16114
Summary:
The option names for `Vary Subjects` are copypasta from the `Add "Re:" Prefix` option. Fix their names to refer to `Vary Subjects` instead.
Fixes T11148
Test Plan: Verify option names for `Vary Subjects` refer to `Add "Re:" Prefix` before apply. Verify they no longer do after apply.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T11148
Differential Revision: https://secure.phabricator.com/D16113
Summary:
Ref T9789. `Transaction` and `Editor` classes are the last major pieces of infrastructure that haven't been fully modularized.
Some of the specific issues are:
- `Editor` classes rely on a bunch of `instanceof` stuff in the base class to pick up transaction types like "subscribe", "projects", etc. Instead, applications should be adding these, and third-party applications should be able to add them.
- Code is spread across `Transaction` and `Editor` classes somewhat oddly. For example, generating old/new values would probably make more sense at the `Transaction` level, but it currently exists at the `Editor` level.
- Both types of classes have a lot of functions based on `switch()` statements, which require a ton of boilerplate and are just generally kind of hard to work with.
This creates classes for each type of transaction, and moves almost all of the logic to them. These classes are simpler and more focused than the old stuff was, and can organize related code better.
This starts inching toward defining `CoreTransactions` for features shared across applications. It only defines the "Create" transaction so far, but at some point I plan to move all the other shared transactions to Core and let them control which objects they're available for.
Test Plan:
- Created pastes with web UI and API.
- Edited all paste properites.
- Archived/activated.
- Verified files got reasonable names.
- Reviewed timeline and feed.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9789
Differential Revision: https://secure.phabricator.com/D16111
Summary:
Ref T11098. Users with at least one setting set correctly fall back to the defaults, but users with no settings at all currently do not.
Make them fall back to global defaults properly.
Test Plan:
- Set global defaults to some non-default setting.
- Completely delete a user's settings.
- `bin/cache purge --purge-all` or `--purge-user`.
- View settings as the user.
- Before change: showed hard-coded defaults instead of global defaults until you save anything.
- After change: properly shows global defaults from the start.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16112
Summary:
Ref T11035. This only fixes half of the issue: comment editing has been fixed, but normal transactions which edit things like descriptions haven't yet.
The normal edits aren't fixed because the "oldValues" are populated too late. The code should start working once they get populated sooner, but I don't want to jump the gun on that since it'll probably have some spooky effects. I have some other transaction changes coming down the pipe which should provide a better context for testing "oldValue" population order.
Test Plan:
- Mentioned `@dog` in a comment.
- Removed `@dog` as a subscriber.
- Edited the comment, adding some unrelated text at the end (e.g., fixing a typo).
- Before change: `@dog` re-added as subscriber.
- After change: no re-add.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11035
Differential Revision: https://secure.phabricator.com/D16108
Summary: Adds a quick edit link to PhamePosts in ApplicationSearch
Test Plan: Review a few searches, click on pencil.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16109
Summary:
When having lots of repos, seeing "all revisions in this project" is hard, and we ended up adding herald rules to basically copy project tags to the revisions on a per-project basis. Adding a "tagged: project" function to the Repositories search field allows users to find differentials within a project.
Fix T10850.
Test Plan: search differentials by tagging project and repository in the Repository field
Reviewers: avivey, epriestley, #blessed_reviewers
Reviewed By: avivey, epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T10850
Differential Revision: https://secure.phabricator.com/D16096
Summary: Forgot to save this file locally. Adds isArchived to same hidden features as isDraft
Test Plan: test mail on archived posts
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16106
Summary: Ref T9897. Adds ability to Archive a Phame Post (only visible under ApplicationSearch).
Test Plan: Archive a post, re-publish it, search for it, archive it again. View Home, Blog, Live pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D16104
Summary: Fixes T11139. We missed this years ago when we moved to PhutilUTF8StringTruncator.
Test Plan: {F1686072}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11139
Differential Revision: https://secure.phabricator.com/D16105
Summary:
Ref T10547. This has been around for a while but I was never able to reproduce it. I caught a repro case in the cluster recently and I think this is the right fix.
We tell Subversion to run `ssh-connect` instead of `ssh` so we can provide options and credentials, by using `SVN_SSH` in the environment. Subversion will sometimes kill the SSH tunnel subprocess aggressively with SIGTERM -- as of writing, you can search for `SIGTERM` in `make_tunnel()` here:
http://svn.apache.org/repos/asf/subversion/trunk/subversion/libsvn_ra_svn/client.c
By default, when a PHP process gets SIGTERM it just exits immediately, without running destructors or shutdown functions. Since destructors/shutdown functions don't run, `TempFile` doesn't get a chance to remove the file.
I don't have a clear picture of //when// Subversion sends SIGTERM to the child process. I can't really get this to trigger locally via `svn`, although I was able to get it to trigger explicitly. So I'm only about 95% sure this fixes it, but it seems likely.
Test Plan:
Locally, I couldn't get this to reproduce "normally" even knowing the cause (maybe Subversion doesn't do the SIGTERM stuff on OSX?) but I was able to get it to reproduce reliabily by adding `posix_kill(getmypid(), SIGTERM);` to the body of the script.
With that added, running the script with `PHABRICATOR_CREDENTIAL=PHID-CDTL-...` in the environment reliably left straggler temporary files.
Adding `declare()` and a signal handler fixed this: the script now runs the `TempFile` destructor and longer leaves the stragglers around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10547
Differential Revision: https://secure.phabricator.com/D16102
Summary:
Ref T11137. This class is removed in D16099. Depends on D16099.
`PhutilURI` now attempts to "just work" with Git-style URIs, so at least in theory we can just delete all of this code and pretend it does not exist.
(I've left "Display URI" and "Effective URI" as distinct, at least for now, because I think the distinction may be relevant in the future even though it isn't right now, and to keep this diff small, although I may go remove one after I think about this for a bit.)
Test Plan:
- Created a new Git repository with a Git URI.
- Pulled/updated it, which now works correctly and should resolve the original issue in T11137.
- Verified that daemons now align the origin to a Git-style URI with a relative path, which should resolve the original issue in T11004.
- Grepped for `PhutilGitURI`.
- Also grepped in `arcanist/`, but found no matches, so no patch for that.
- Checked display/conduit URIs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11137
Differential Revision: https://secure.phabricator.com/D16100
Summary:
Ref T11137. This class is removed in D16099. Depends on D16099.
`PhutilURI` now attempts to "just work" with Git-style URIs, so at least in theory we can just delete all of this code and pretend it does not exist.
(I've left "Display URI" and "Effective URI" as distinct, at least for now, because I think the distinction may be relevant in the future even though it isn't right now, and to keep this diff small, although I may go remove one after I think about this for a bit.)
Test Plan:
- Created a new Git repository with a Git URI.
- Pulled/updated it, which now works correctly and should resolve the original issue in T11137.
- Verified that daemons now align the origin to a Git-style URI with a relative path, which should resolve the original issue in T11004.
- Grepped for `PhutilGitURI`.
- Also grepped in `arcanist/`, but found no matches, so no patch for that.
- Checked display/conduit URIs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11137
Differential Revision: https://secure.phabricator.com/D16100
Summary:
Ref T7643. When a large block of prose text is edited (like a wiki page), summarize the diff when sending mail.
For now, I'm still showing the whole thing in the web UI, since it's a bit more manageable there.
Also try to fix newlines in Airmail.
Test Plan:
This web diff:
{F1682591}
..became this mail diff:
{F1682592}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16098
Summary:
Ref T8510. Sort prefix matches above non-prefix matches, so that "Ape Discovery" does not match "discovery" better than "Discovery".
Sort functions last.
Rename function internal strings so they don't get over-promoted the prefix-match rules.
Add kind of a hack to get "Project X" sorting above all the "Project X (Milestone 1)" results.
Test Plan:
Created "Ape Discovery", "Baboon Discovery", "Chimpanzee Discovery", etc.
Main project now sorts above milestones:
{F1681773}
Prefix matches now sort above other matches:
{F1681774}
Function results (rarely used) are now less prominent:
{F1681775}
Better function results here:
{F1681776}
More function results:
{F1681777}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8510
Differential Revision: https://secure.phabricator.com/D16094
Summary: Ref T11120. If this works, I'll just remove this option completely.
Test Plan: ¯\_(ツ)_/¯
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11120
Differential Revision: https://secure.phabricator.com/D16095
Summary: "All Tasks" is bad in the long run and not clearly better for new installs.
Test Plan: Created a new smiple template, saw open tasks only.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16093
Summary: Ref T10227. When we perform `git` http operations (fetch, mirror) check if we should use a proxy; if we should, set `http_proxy` or `https_proxy` in the environment to make `git` have `curl` use it.
Test Plan:
- Configured a proxy extension to run stuff through a local instance of Charles.
- Ran `repository pull` and `repository mirror`.
- Saw `git` HTTP requests route through the proxy.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10227
Differential Revision: https://secure.phabricator.com/D16092
Summary: Ref T11123. This implements a very basic skeleton for modern revision search.
Test Plan: Viewed and executed Conduit API method.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11123
Differential Revision: https://secure.phabricator.com/D16089
Summary: Ref T6523. Allows you to click stuff instead of using drag-and-drop.
Test Plan: On iOS simulator, created and updated a mock.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6523
Differential Revision: https://secure.phabricator.com/D16088
Summary: Fixes T10886. This should get more formal some day, but just fix it for now.
Test Plan: Reloaded mock with other unpublished draft inlines, saw accurate count.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10886
Differential Revision: https://secure.phabricator.com/D16087
Summary: Fixes T11115, but unclear how to test this. I think I've asked this in the past.
Test Plan:
- Visit Applications -> Ponder
- Configure external email
- Test External Email
- See new Question
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11115
Differential Revision: https://secure.phabricator.com/D16084
Summary: Ref T4280. At some point (probably D15732) we started getting anchor parsing wrong. Just pop the anchor off before doing all the logic, then put it back on at the end.
Test Plan:
Tested various forms like:
```
[[ x ]]
[[ x | z ]]
[[ x#y | z ]]
[[ ./x#y | z ]]
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4280
Differential Revision: https://secure.phabricator.com/D16083
Summary: Fixes T11113. On the 2nd+ page, we could end up with an ambiguous `id` WHERE clause because we don't define a primary table alias on this query. Define one.
Test Plan:
Changed SearchEngine to return pages of size 5, searched for my threads, toggled to second page, no exception.
Used DarkConsole to examine that second-page query, saw that it had `thread.id` explicitly instead of `id` implicitly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11113
Differential Revision: https://secure.phabricator.com/D16080
Summary:
Via HackerOne. This page fatals if accessed directly while logged out.
The "shouldRequireLogin()" check is wrong; this is a logged-in page.
Test Plan:
Viewed the page while logged out, no more fatal.
Faked my way through the actual verification flow.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16077
Summary:
Ref T6916. Current rules tend to make videos gigantic. Just embed them at actual size, scaling them down if they're too big to fit.
Browsers generally provide some kind of "expand / fullscreen" element automatically anyway.
Test Plan: Viewed videos locally, saw them sized a little more reasonably.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6916
Differential Revision: https://secure.phabricator.com/D16076
Summary: Fixes T11107. The URI change here meant we were dropping the "key" parameter, which allows you to set a new password without knowing your old one.
Test Plan: Reset password, didn't need to provide old one anymore.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11107
Differential Revision: https://secure.phabricator.com/D16075
Summary: Ref T6916. Added video to remarkup using D7156 as reference.
Test Plan:
- Viewed video files (MP4, Ogg) in Safari, Chrome, Firefox (some don't work, e.g., OGG in Safari, but nothing we can really do about that).
- Used `alt`.
- Used `autoplay`.
- Used `loop`.
- Used `media=audio`.
- Viewed file detail page.
Reviewers: nateguchi2, chad, #blessed_reviewers
Reviewed By: chad, #blessed_reviewers
Subscribers: asherkin, ivo, joshuaspence, Korvin, epriestley
Tags: #remarkup
Maniphest Tasks: T6916
Differential Revision: https://secure.phabricator.com/D11297
Summary:
Fixes T10402.
I tried about 50 variations on the wording and notification layout, this seemed by far the most reasonable.
Didn't implement a way to ignore the warning, which might be required - but figured this is serious and broken enough while being completely invisible 99% of the time that it's worth shouting about.
Test Plan: Messed around with $_SERVER['HTTPS'] on the server side and client_uri on the client side - saw reasonable results in all combinations.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T10402
Differential Revision: https://secure.phabricator.com/D16064
Summary:
Ref T11098. This primarily fixes Conduit calls to `*.edit` methods failing when trying to access user preferences.
(The actual access is a little weird, since it seems like we're building some UI stuff inside a policy query, but that's an issue for another time.)
To fix this, consolidate the "we're about to run some kind of request with this user" code and run it consistently for web, conduit, and SSH sessions.
Additionally, make sure we swap things to the user's translation.
Test Plan:
- Ran `maniphest.edit` via `arc call-conduit`, no more settings exception.
- Set translation to ALL CAPS, got all caps output from `ssh` and Conduit.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16066
Summary:
Ran into this while fixing T11098#179088.
The "Transaction Type" details in the conduit autogenerated documentation for `*.edit` endpoints still wraps incorrectly.
Test Plan: Purged remarkup cache, reloaded page, got full-width text.
Reviewers: avivey, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16065
