Summary: Fixes T5703. These have been unused in production for a while and the new stuff seems good.
Test Plan: Mostly `grep`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5703
Differential Revision: https://secure.phabricator.com/D12949
Summary: Converts most all tables to be directly set via `setTable` to an ObjectBox. I think this path is more flexible design wise, as we can change the box based on children, and not just CSS. We also already do this with PropertyList, Forms, ObjectList, and Header. `setCollapsed` is added to ObjectBox to all children objects to bleed to the edges (like diffs).
Test Plan: I did a grep of `appendChild($table)` as well as searches for `PHUIObjectBoxView`, also with manual opening of hundreds of files. I'm sure I missed 5-8 places. If you just appendChild($table) nothing breaks, it just looks a little funny.
Reviewers: epriestley, btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12955
Summary: Remove the `PhabricatorDefaultSearchEngineSelector` class. This is quite similar to D12053.
Test Plan: Went to `/view/PhabricatorSearchApplication/` and saw the storage engine configuration. Set `search.elastic.host` and saw the highlighted storage engine change.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12670
Summary: Ref T6403. This was actually simple stuff.
Test Plan: changed the edit policy of a paste. changed the edit and join policy of a phame blog.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12933
Summary:
Fixes T6956. Before this change, we called PhabricatorUser::getOmnipotentUser in the various delete methods to query the data. Now, we use $engine->getViewer(), since its always a good thing to have less calls to PhabricatorUser::getOmnipotentUser thrown around the codebase.
I used the "codemod" tool to audit the existing calls to PhabricatorDestructorEngine (all of them) so ostensibly this gets all the spots. If I missed something though, its still going to work, so this change is very low risk.
Test Plan: ./bin/remove destroy P1; visit P1 and get a 404
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6956
Differential Revision: https://secure.phabricator.com/D12866
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12806
Summary:
Ref T7707. The general form of this can probably be refined somewhat over time as we have more use cases.
I put this cache on the user object itself because we essentially always need this data and it's trivial to invalidate the cache (we can do it implicilty during reads).
Also fix an issue with short, wide images not thumbnailing properly after recent changes.
Test Plan:
- Loaded some pages; saw caches write; saw good pictures.
- Reloaded; saw cache reads; saw good pictures.
- Changed profile picture; saw immediate update.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7707
Differential Revision: https://secure.phabricator.com/D12826
Summary:
Ref T7707. Fixes T7879. Fixes T4406. When creating profile images:
- Use the new transforms;
- mark them as "profile" images so they're forced to the most-open policies.
Test Plan:
- Set restrictive default file policies.
- Changed profile picture, project pictures, etc. Verified they were visible to logged-out users.
- Registered via OAuth.
- Updated a Conpherence thread image.
- Browsed around looking for profile images, fixed sizing on everything I could find.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7879, T7707, T4406
Differential Revision: https://secure.phabricator.com/D12821
Summary:
Ref T7707. This ends up being sort of complicated: to support 100x100 images in T4406, we need to scale small images //up// so they look OK when we scale them back down with `background-size` in CSS.
The rest of it is mostly straightforward.
Test Plan:
- Did an OAuth handshake and saw a scaled-up, scaled-down profile picture that looked correct.
- Used Pholio, edited pholio, embedded pholio.
- Uploaded a bunch of small/weird/big images and regenerated all their transforms.
- Uploaded some text files into Pholio.
- Grepped for removed methods, etc.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7707
Differential Revision: https://secure.phabricator.com/D12818
Summary: Ref T7707. Move the 220px (file uploads) and 100px (Pholio thumbgrid) previews over to the new stuff.
Test Plan: Uploaded a bunch of images to remarkup and Pholio; they generated reasonable results in the web UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7707
Differential Revision: https://secure.phabricator.com/D12814
Summary: Ref T7707. For animated GIFs, use imagemagick if it is available.
Test Plan: Generated small versions of a bunch of different GIFs.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7707
Differential Revision: https://secure.phabricator.com/D12813
Summary: Ref T7707. These transforms have a single maximum dimension instead of fixed X and Y dimensions.
Test Plan: Transformed a bunch of files with different sizes/aspect ratios, got sensible results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7707
Differential Revision: https://secure.phabricator.com/D12812
Summary:
Ref T7707. Ref T4406. Ref T2479. This implements the profile-style (fixed width and height) transforms in a modern way.
- Added a "regnerate" feature to the support UI to make testing easier and surface errors.
- Laboriously check errors from everything.
- Fix the profile thumbnailing so it crops properly instead of leaving margins.
- Also defuses the "gigantic white PNG" attack.
This doesn't handle the imagemagick case (for animated GIFs) yet.
Test Plan:
- Uploaded a variety of wide/narrow/small/large files and converted them into sensible profile pictures.
- Tried to thumbnail some text files.
- Set the pixel-size and file-size limits artificially small and hit them.
- Used "regenerate" a bunch while testing the rest of this stuff.
- Verified that non-regenerate flows still produce a default/placeholder image.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4406, T2479, T7707
Differential Revision: https://secure.phabricator.com/D12811
Summary: Ref T7707. Fixes T4724. I misread the report on T4724; this is trivial. We're just reading the wrong properties in setting "width" and "height" attributes, the actual thumbnailing logic is fine.
Test Plan: Uploaded image from T4724, saw it have a proper aspect ratio.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: spicyj, epriestley
Maniphest Tasks: T4724, T7707
Differential Revision: https://secure.phabricator.com/D12810
Summary:
Ref T7707. Ref T2479. Ref T5258.
The thumbnailing code is some of the only code in the codebase which doesn't use exceptions to handle errors. I'm going to convert it to use exceptions; make sure they do something reasonable at top level.
Strategy here is:
- By default, we just fall back to a placeholder image if anything goes wrong.
- Later, I'll likely add a "debug" workflow from the new "Transforms" UI which will surface the specific exception instead (the code can't really raise any interesting exceptions right now).
Test Plan: Faked an exception and saw some reasonable default images.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5258, T2479, T7707
Differential Revision: https://secure.phabricator.com/D12809
Summary:
Ref T7707. Available transforms are currently relatively hard-coded and don't really have any support UI.
Modularize them so we can build some support UI.
This doesn't actually //use// any of the new stuff yet: I want to make a clean cutover once I fix the aspect ratio stuff so I can pick up a cachekey/URI change as a side effect.
Test Plan: {F400524}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T7707
Differential Revision: https://secure.phabricator.com/D12808
Summary: Fixes T7480, File names should be editable and the event should show up in feed.
Test Plan: Upload a file, view file details, edit file, change file name by adding a space and a word to the name, save changes, file name should retain space and not normalize the name, file details should show the edit event, install feed should correctly show an event for the action.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7480
Differential Revision: https://secure.phabricator.com/D12561
Summary: Fixes T7685. This required making the global drag and drop behavior able to "uninstall" itself so to speak, and then it re-installs it self as necessary.
Test Plan:
Did the following all successfully
- uploaded a file to homepage
- homepage -> differential -- no way to upload via drag and drop
- homepage -> differential -> homepage -- uploaded a file
- homepage -> differential -> browser back button to homepage -- uploaded a file
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7685
Differential Revision: https://secure.phabricator.com/D12534
Summary:
Ref T4100. Ref T5595. These functions are trivial for now, but move us toward being able to define more default query behavior by default.
Future changes will give these methods meaningful, nontrivial behaviors.
Test Plan: `arc unit --everything`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5595, T4100
Differential Revision: https://secure.phabricator.com/D12454
Summary:
Ref T4100. Ref T5595.
To support a unified "Projects:" query across all applications, a future diff is going to add a set of "Edge Logic" capabilities to `PolicyAwareQuery` which write the required SELECT, JOIN, WHERE, HAVING and GROUP clauses for you.
With the addition of "Edge Logic", we'll have three systems which may need to build components of query claues: ordering/paging, customfields/applicationsearch, and edge logic.
For most clauses, queries don't currently call into the parent explicitly to get default components. I want to move more query construction logic up the class tree so it can be shared.
For most methods, this isn't a problem, but many subclasses define a `buildWhereClause()`. Make all such definitions protected and consistent.
This causes no behavioral changes.
Test Plan: Ran `arc unit --everything`, which does a pretty through job of verifying this statically.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, hach-que, epriestley
Maniphest Tasks: T4100, T5595
Differential Revision: https://secure.phabricator.com/D12453
Summary:
Ref T7803. Ref T5873. I want to drive Conduit through more shared infrastructure, but can't currently add parameters automatically.
Put a `getX()` around the `defineX()` methods so the parent can provide default behaviors.
Also like 60% of methods don't define any special error types; don't require them to implement this method. I want to move away from this in general.
Test Plan:
- Ran `arc unit --everything`.
- Called `conduit.query`.
- Browsed Conduit UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T5873, T7803
Differential Revision: https://secure.phabricator.com/D12380
Summary:
Ref T7803. Some Query subclasses implement getPagingColumn() in a trivial way, usually to provide a table alias.
Formalize the concept of a primary table alias, and remove obsoleted getPagingColumn() implementations.
Test Plan: Issued affected queries.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12356
Summary: These arrays looks a little odd, most likely due to the autofix applied by `ArcanistXHPASTLinter::LINT_ARRAY_SEPARATOR`. See D12296 in which I attempt to improve the autocorrection from this linter rule.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12281
Summary:
Fixes T7199. This still isn't a shining example of perfect code, but the raw amount of copy/paste is much lower than it used to be.
- Reduce code duplication between existing receivers.
- Expose receiving objects in help menus where appropriate.
- Connect some "TODO" receivers.
Test Plan:
- Sent mail to every supported object type.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12249
Summary: Ref T7199. Everyone can have a mail command! You can have a mail command! You can have a mail command! Mail commands for everyone!
Test Plan: Used `bin/mail receive-test` to issue commands against files and pastes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12238
Summary: Ref T7199. Half of these aren't even reachable, but make some progress toward reducing the amount of nonsense and garbage in mail handling.
Test Plan: Tested all reachable handlers with `bin/mail receive-test`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12237
Summary:
Ref T7199. This prepares for an exciting new world of more powerful "!action" commands. In particular:
- We parse multiple commands per mail.
- We parse command arguments (these are currently not used).
- We parse commands at the beginning or end of mail.
Additionally:
- Do a quick modernization pass on all handlers.
- Break legacy compatibility with really hacky Facebook stuff (see T1992). They've theoretically been on notice for a year and a half, and their setup relies on calling very old reply handler APIs directly.
- Some of these handlers had some copy/paste fluff.
- The Releeph handler is unreachable, but fix it //in theory//.
Test Plan:
- Sent mail to a file; used "!unsubscribe".
- Sent mail to a legalpad document; used "!unsubscribe".
- Sent mail to a task; used various "!close", "!claim", "!assign", etc.
- Sent mail to a paste.
- Sent mail to a revision; used various "!reject", "!claim", etc.
- Tried to send mail to a pull request but it's not actually reachable.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12230
Summary:
Ref T7199. Although this is useful for discovery, it's un-useful enough that we already have an option to disable it, and most applications do not provide any meaningful instructions.
Throwing it away makes it easier to move forward and lets us get rid of a config option.
This is becoming a more advanced/power-user feature anyway, and the new syntax will be significantly more complex and hard to explain with a one-liner. I'm currently thinking that I'll maybe make the "help" menu a dropdown and give it some options like:
+---+
| O |
+---+---------------------+
| Maniphest Documentation |
| Maniphest Email Actions |
+-------------------------+
Then you click the "Email Actions" thing and get a runtime-derived list of available options. Not sure if I'll actually build that, but I think we can fairly throw the in-mail instructions away even if we don't go in that specific direction.
Test Plan: Grepped for `replyHandlerInstructions`, got no hits.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12229
Summary: Ref T7199. Guess no one has ever tried to reply to file mail.
Test Plan: Used `bin/mail receive-test` to send mail to files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12228
Summary: Ref T7689. Ref T4100. This advances the goals of removing `loadViewerHandles()` (only 67 callsites remain!) and letting tokenizers some day take token functions like `viewer()` and `members(differential)`.
Test Plan:
- Sent a new message; used "To".
- I simplified the cancel URI construction slightly because it's moot in all normal cases.
- Edited a thread; used "Add Participants".
- Searched rooms; used "Participants".
- Searched countdowns; used "Authors".
- Created a diff; used "Repository".
- Edited a revision; edited "Projects"; edited "Reveiwers"; edited "Subscribers".
- Searched for revisions; edited "responsible users"; "authors"; "reviwers"; "subscribers"; "repositories".
- Added revision comments; edited "Add Reveiwers"; "Add Subscribers".
- Commented on a commit; edited "Add Auditors"; "Add subscribers".
- Edited a commit; edited "Projects".
- Edited a repository; edited "Projects".
- Searched feed, used "include Users"; "include Proejcts".
- Searched files, used "authors".
- Edited initiative; edited "Projects".
- Searched backers; used "Backers".
- Searched initiatives; used "Owners".
- Edited build plans; edited "Run Command".
- Searched Herald; used "Authors".
- Added signature exemption in Legalpad.
- Searhced legalpad; used "creators"; used "contributors".
- Searched signatures; used "documents"; used "signers".
- Created meme.
- Searched macros; used "Authors".
- Used "Projects" in Maniphest reports.
- Used Maniphest comment actions.
- Edited Maniphest tasks; edited "Assigned To"; edited "CC"; edited "projects".
- Used "parent" in Maniphest task creation workflow.
- Searched for projects; used "assigned to"; "in any projec"; "in all projects"; "not in projects"; "in users' projects"; "authors"; "subscribers".
- Edited Maniphest bug filing domains, used "Default Author".
- Searched for OAuth applications, used "Creators".
- Edited Owners pacakge; edited "Primary Owner"; edited "Owners".
- Searched for Owners packages; used "Owner".
- OMG this UI is OLD
- Edited a paste; edited "Projects".
- Searched for paste; used "Authors".
- Searched user activity log; used "Actors"; used "Users".
- Edited a mock; edited "Projects"; edited "CC".
- Searched for mocks; used "Authors".
- Edited Phortune account; edited "Members".
- Edited Phortune merchant account; edited "Members".
- Searched Phrequent; used "Users".
- Edited Ponder question; sued "projects".
- Searched Ponder; used "Authors"; used "Answered By".
- Added project members.
- Searched for projects; used "Members".
- Edited a Releeph product; edited "Pushers".
- Searched pull requests; searched "Requestors".
- Edited an arcanist project; used "Uses Symbols From".
- Searhced push logs; used "Repositories"; used "Pushers".
- Searched repositories; used "In nay project".
- Used global search; used Authors/owners/Subscribers/In Any Project.
- Edited a slowvote; used "Projects".
- Searched slovotes; used "Authors".
- Created a custom "Users" field; edited and searched for it.
- Made a whole lot of typos in this list. ^^^^^^
Did not test:
- Lint is nontrivial to test locally, I'll test it in production.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100, T7689
Differential Revision: https://secure.phabricator.com/D12224
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
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
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
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
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
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
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
Summary:
Ref T7149. Currently, global drag and drop always uses the most open visibility policy on the install. This was appropriate before the application preference was introduced, but default to the application preference now.
In particular, this supports a default value of "Administrators" in the Phacility cluster.
Also simplify/clean up some code.
Test Plan:
- Set application default policy to "Adminstrators".
- Uploaded file via drag-and-drop, saw "administrators" policy.
- Uploaded file via `arc upload`, saw "administrators" policy.
- Saw better URI for a text file upload after patch.
- Uploaded file via drag-and-drop-to-textarea, saw "only viewer" policy.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12093
Summary:
Ref T7149. We can simplify configuration somewhat by removing the upload limit setting, now that we support arbitrarily large files.
- Merge configuration documentation.
- Tell users to set things to at least 32MB. This is 8MB maximum one-shot file + 4x headroom. Chunk sizes are 4MB.
Test Plan:
- Faked all the setup warnings.
- Read documentation.
- Uploaded some files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12083
Summary: Ref T7149. This works now, so enable it.
Test Plan:
- Uploaded large and small files in Firefox, Safari and Chrome.
- Uploaded large files with `arc upload`.
- Stopped/resumed large files with all clients.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12079
Summary: Ref T7149. This was just to make testing easier, but chunking substantially works now.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12076
Summary: Ref T7149. Return a real iterator from the Chunk engine, which processes chunks sequentially.
Test Plan:
This is a bit hard to read, but shows the underlying chunks being accessed one at a time and only some being accessed when requesting a range of a file:
```
$ ./bin/files cat F878 --trace --begin 100 --end 256
...
>>> [10] <query> SELECT * FROM `file_storageblob` WHERE `id` = 85
<<< [10] <query> 240 us
better software.
Phabricat>>> [11] <query> SELECT * FROM `file_storageblob` WHERE `id` = 84
<<< [11] <query> 205 us
or includes applications for:
>>> [12] <query> SELECT * FROM `file_storageblob` WHERE `id` = 83
<<< [12] <query> 226 us
- reviewing and auditing source>>> [13] <query> SELECT * FROM `file_storageblob` WHERE `id` = 82
<<< [13] <query> 203 us
code;
- hosting and browsing >>> [14] <query> SELECT * FROM `file_storageblob` WHERE `id` = 81
<<< [14] <query> 231 us
repositories;
- tracking bugs;
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12073
Summary:
Ref T7149. This still buffers the whole file, but is reaaaaal close to not doing that.
Allow Responses to be streamed, and rewrite the range stuff in the FileResponse so it does not rely on having the entire content available.
Test Plan:
- Artificially slowed down downloads, suspended/resumed them (works in chrome, not so much in Safari/Firefox?)
- Played sounds in Safari/Chrome.
- Viewed a bunch of pages and files in every browser.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12072
Summary: Ref T7149. A couple diffs down the line, this will let us emit chunked files without doing all the work up front or holding the entire file in RAM.
Test Plan:
(Some newlines added for clarity.)
```
$ ./bin/files cat F942
ABCDEFGHIJKLMNOPQRSTUVWXYZ
$ ./bin/files cat F942 --begin 1
BCDEFGHIJKLMNOPQRSTUVWXYZ
$ ./bin/files cat F942 --end 10
ABCDEFGHIJ
$ ./bin/files cat F942 --begin 3 --end 5
DE
$
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12071
Summary: Ref T7149. We can't compute hashes of large files efficiently, but we can resume uploads by the same author, with the same name and file size, which are only partially completed. This seems like a reasonable heuristic that is unlikely to ever misfire, even if it's a little magical.
Test Plan:
- Forced chunking on.
- Started uploading a chunked file.
- Closed the browser window.
- Dropped it into a new window.
- Upload resumed //(!!!)//
- Did this again.
- Downloaded the final file, which successfully reconstructed the original file.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, chad, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12070
Summary:
Ref T7149. This adds chunking support to drag-and-drop uploads. It never activates right now unless you hack things up, since the chunk engine is still hard-coded as disabled.
The overall approach is the same as `arc upload` in D12061, with some slight changes to the API return values to avoid a few extra HTTP calls.
Test Plan:
- Enabled chunk engine.
- Uploaded some READMEs in a bunch of tiny 32 byte chunks.
- Worked out of the box in Safari, Chrome, Firefox.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12066
Summary:
Ref T7149. This flags allocated but incomplete files and doesn't explode when trying to download them.
Files are marked complete when the last chunk is uploaded.
I added a key on `<authorPHID, isPartial>` so we can show you a list of partially uploaded files and prompt you to resume them at some point down the road.
Test Plan: Massaged debugging settings and uploaded README.md very slowly in 32b chunks. Saw the file lose its "Partial" flag when the last chunk finished.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12063
Summary:
Ref T7149. This makes debugging some of this stuff a bit easier by removing the HTTP part in the middle.
Particularly, I anticipate having this stream data chunk-by-chunk in the near future.
Test Plan: Ran `files cat F23`, got output.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12062
Summary:
Ref T7149. This isn't complete and isn't active yet, but does basically work. I'll shore it up in the next few diffs.
The new workflow goes like this:
> Client, file.allocate(): I'd like to upload a file with length L, metadata M, and hash H.
Then the server returns `upload` (a boolean) and `filePHID` (a PHID). These mean:
| upload | filePHID | means |
|---|---|---|
| false | false | Server can't accept file.
| false | true | File data already known, file created from hash.
| true | false | Just upload normally.
| true | true | Query chunks to start or resume a chunked upload.
All but the last case are uninteresting and work like exising uploads with `file.uploadhash` (which we can eventually deprecate).
In the last case:
> Client, file.querychunks(): Give me a list of chunks that I should upload.
This returns all the chunks for the file. Chunks have a start byte, an end byte, and a "complete" flag to indicate that the server already has the data.
Then, the client fills in chunks by sending them:
> Client, file.uploadchunk(): Here is the data for one chunk.
This stuff doesn't work yet or has some caveats:
- I haven't tested resume much.
- Files need an "isPartial()" flag for partial uploads, and the UI needs to respect it.
- The JS client needs to become chunk-aware.
- Chunk size is set crazy low to make testing easier.
- Some debugging flags that I'll remove soon-ish.
- Downloading works, but still streams the whole file into memory.
- This storage engine is disabled by default (hardcoded as a unit test engine) because it's still sketchy.
- Need some code to remove the "isParital" flag when the last chunk is uploaded.
- Maybe do checksumming on chunks.
Test Plan:
- Hacked up `arc upload` (see next diff) to be chunk-aware and uploaded a readme in 18 32-byte chunks. Then downloaded it. Got the same file back that I uploaded.
- File UI now shows some basic chunk info for chunked files:
{F336434}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12060
Summary:
Fixes T5843. File storage engines use a very old "selector" mechanism which makes them difficult to extend.
This mechanism predates widespread use of `PhutilSymbolLoader` to discover available implementations at runtime. Runtime discovery has generally proven more flexible and easier to use than explicit selection (although it sometimes needs more UI to support it in cases where order or enabled/disabled flags can not be directly determined).
Use a modern runtime discovery mechanism instead of an explicit selector. This might break any installs which subclassed the `Selector`, but I believe almost no such installs exist, and they'll receive a meaningful exception upon upgrading (any custom engines will no longer implement all of the required methods).
Looking forward, this modernizes infrastructure to prepare for new "virtual" chunked-storage engines, with the eventual goal of supporting very large file uploads and data import into the Phacility cluster.
This uses D12051 to add UI to make it easier to understand the state of storage engines.
Test Plan:
Used new UI panel to assess storage engines:
{F336270}
- Uploaded a small file, saw it go to MySQL engine.
- Uploaded a larger file, saw it go to S3 engine.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5843
Differential Revision: https://secure.phabricator.com/D12053
Summary: This makes macros and memes grow to 100% of their container //at most//, instead of showing a scrollbar. This is useful for overly large macros, smaller spaces like Feed and Conpherences, and Inline Comments. Fixes T7528
Test Plan: Tested a very large macro, a very large meme, and a very very tiny macro. It looks like memes get cached though, unsure if we should clean them up or just leave them
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7528
Differential Revision: https://secure.phabricator.com/D12045
Summary: Fixes T7398. Previously, we would redirect to get a token and then redirect back to make use of it, but lose "download" in the process, and thus not get the correct "Content-Disposition" header.
Test Plan: Clicked "Download" on a lightboxed file.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7398
Differential Revision: https://secure.phabricator.com/D11915
Summary:
Fixes T7379. Currently, builtin files generate with a "users" view policy even if an install is public.
Because these files TTL after 7 days, there's no migration here. Installs won't see the fix actually happen for up to 7 days after updating, though.
Test Plan:
- Deleted a builtin.
- Loaded projects page to regenerate it.
- Saw new builtin had most open policy and was marked as a builtin.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7379
Differential Revision: https://secure.phabricator.com/D11917
Summary:
This makes thumbnail URIs work on instanced, CDN'd installs like Phacility cluster instances.
Some of these transforms can proabably be removed, but the underlying code to generate the transform should be cleaned up too and we have some other tasks filed elsewhere about this anyway.
Test Plan: CDN'd local install now loads thumbnails properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11719
Summary: Adds core and apps grouping to configuration options, makes it somewhat easier to browse config options.
Test Plan: Set each option, review list. Breakdown is nearly 50/50 apps/core.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11722
Summary:
The generated HTML is like `<p>some text <div …>…</div> more text</p>`, and HTML `<p/>` tags may not contain block content like `<div/>` tags. Browsers actually parse this as if it was `<p>some text </p><div …>…</div> more text<p></p>` (sic).
The layout CSS class already has `display: inline` set, but this is not sufficient. Browser's HTML parser doesn't care what CSS rules will be applied, it only deals with the meanings of tags.
Fixes T7201.
Test Plan:
Verify that the following displays the image inline:
`some text {Fnnn,layout=inline} more text`
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Projects: #remarkup
Maniphest Tasks: T7201
Differential Revision: https://secure.phabricator.com/D11706
Summary: Fixes T7118. This does the basic "filter the list" thing, though it ends up being a little manual since I guess this hasn't come up before? There is also potential weird behavior if the user was using an app and lost access to it - they will have nothing selected on edit - but I think this is actually correct behavior in this circumstance.
Test Plan:
used a user who couldn't get access to the "quick create" apps and noted that the dropdown list on dashboard panel create was missing the expected engines
ran `arc unit --everything` to verify abstract method implemented everywhere
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7118
Differential Revision: https://secure.phabricator.com/D11687
Summary:
This allows us to CDN the cluster.
General problem is that we can't easily give each instance its own CDN URI (`giraffe.phcdn.net`) in Cloudfront, because it requires that you enumerate all aliases (and there's a limit of 100) and depends on SNI (a newish feature of SSL which allows one server to serve multiple certificates, but which doesn't have full support everywhere yet).
It's //possible// that we could eventually work around this, or use Cloudflare instead (which has a different model that seems like a slightly easier fit for CDN-domain-per-instance), but I don't want to sink a ton of work into this and want to keep things on AWS insofar as we reasonably can.
The easiest way to fix this is just to put the instance identity into URIs, then read it out when handling CDN requests. This has no effect on installs without cluster instance configuration, which is all of them except ours.
It's also slightly desirable to share this stuff, since we get to share the cache for static resources, which are always identical across instances.
So requests go from the Cloudfront gateway ("xyz.cloudfront.com") to the LB with a hard-coded instance name ("cdn.phacility.com"), which gets them routed to a balanced web machine. The web machine picks the correct instance name out of the URI, acts as that instance, and does the correct thing.
The messiest part of this is that we need "cdn.phacility.com" to be a real instance so it can serve static resources, but that's not a big deal. We have a few other hard-codes which have to be real resources for now, like we must have a merchant named "Phacility".
Test Plan:
- Viewed files with `security.alternate-file-domain` off (i.e., no file tokens).
- Viewed pages and files with `security.alternate-file-domain` on. Saw correct resource behavior, @isntance generation of URIs, and correct token redirect behavior for files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11668
Summary: This sets an icon for each config, makes it easier to scan.
Test Plan:
Reload Config page, see all new icons
{F281089}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11619
Summary: Fixes T3404 (post D11565), fixes T5952. This infrastructure has been getting deployed against Maniphest and its time to get these other two applications going on it.
Test Plan: created an email address for paste and used `./bin/mail receive-test` ; a paste was successfully created
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5952, T3404
Differential Revision: https://secure.phabricator.com/D11570
Summary: Select a similar or better FontAwesome icon to represent each application
Test Plan: Visual inspection
Reviewers: epriestley, btrahan
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11489
Summary:
Taking a pass at revamping the edit pages in Projects. Specifically:
- Remove EditMainController
- Move actions from EditMain to Profile
- Move properties from EditMain to Profile
- Move timeline from EditMain to Profile
- Move Open Tasks from Profile to sidenavicon
- Add custom icons and colors to timeline
Feel free to bang on this a bit and give feedback, feels generally correct to me.
Test Plan: Edit everything I could on various projects. Check links, timelines, actions.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11421
Summary: Ref T6822.
Test Plan: `grep`. This method is only called from within `PhutilArgumentWorkflow::__construct`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11415
Summary: Ref T6822. This method needs to be `public` because it is called from `PhabricatorApplicationSearchController::buildApplicationMenu()`.
Test Plan: I wouldn't expect //increasing// method visibility to break anything.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11416
Summary: Ref T5752, moves mobile action menus to the object box instead of crumbs.
Test Plan: View action menus at tablet, desktop, and mobile break points. Verify clicking buttons works as expected opening menu.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5752
Differential Revision: https://secure.phabricator.com/D11340
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within `PhabricatorController` subclasses.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11241
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `PhabricatorApplicationSearchEngine` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11242
Summary: Modernize remaining edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: Browsed around and performed various actions include subscribing, unsubscribing and watching.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11116
Summary: Show the php.ini setting for max upload or alterantively the in phabricator configured one. Fixes T6663
Test Plan: changed php.ini and alternatively phabricator file upload size settings to minimal values and try to upload a larger file
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, btrahan, chad, epriestley
Maniphest Tasks: T6663
Differential Revision: https://secure.phabricator.com/D11031
Summary: Removes the docs sprite in Conpherence with FontAwesome, adds additional icons. Unsure what happens if someone customized this config option.
Test Plan: Added images and files to a Conpherence, saw new icons.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11028
Summary: Fixes T6693.
Test Plan:
Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!
Successfully "showed older changes" in Maniphest too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6693
Differential Revision: https://secure.phabricator.com/D10931
Summary: **WIP** This adds default capability to the Files application
Test Plan: Set default to public, go to Files page, see public preset. Upload File. Doesn't work.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6564
Differential Revision: https://secure.phabricator.com/D10888
Summary:
Ref T5702. This is a forward-looking change which provides some very broad API improvements but does not implement them. In particular:
- Controllers no longer require `$request` to construct. This is mostly for T5702, directly, but simplifies things in general. Instead, we call `setRequest()` before using a controller. Only a small number of sites activate controllers, so this is less code overall, and more consistent with most constructors not having any parameters or effects.
- `$request` now offers `getURIData($key, ...)`. This is an alternate way of accessing `$data` which is currently only available on `willProcessRequest(array $data)`. Almost all controllers which implement this method do so in order to read one or two things out of the URI data. Instead, let them just read this data directly when processing the request.
- Introduce `handleRequest(AphrontRequest $request)` and deprecate (very softly) `processRequest()`. The majority of `processRequest()` calls begin `$request = $this->getRequest()`, which is avoided with the more practical signature.
- Provide `getViewer()` on `$request`, and a convenience `getViewer()` on `$controller`. This fixes `$viewer = $request->getUser();` into `$viewer = $request->getViewer();`, and converts the `$request + $viewer` two-liner into a single `$this->getViewer()`.
Test Plan:
- Browsed around in general.
- Hit special controllers (redirect, 404).
- Hit AuditList controller (uses new style).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10698
Summary:
Ref T1191. Now that the whole database is covered, we don't need to do as much work to build expected schemata. Doing them database-by-database was helpful in converting, but is just reudndant work now.
Instead of requiring every application to build its Lisk objects, just build all Lisk objects.
I removed `harbormaster.lisk_counter` because it is unused.
It would be nice to autogenerate edge schemata, too, but that's a little trickier.
Test Plan: Database setup issues are all green.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10620
Summary:
Ref T1191.
- Adds definitions for missing keys and keys with wrong uniqueness. Generally, I defined these before fixing the key query to actually pull all keys and support uniqueness.
- Moves "key uniqueness" to note severity; this is fixable (probably?) and there are no remaining issues.
- Moves "Missing Key" to note severity; missing keys are fixable and all remaining missing keys are really missing (either missing edge keys, or missing PHID keys):
{F210089}
- Moves "Surplus Key" to note seveirty; surplus keys are fixable all remaining surplus keys are really surplus (duplicate key in Harbormaster, key on unused column in Worker):
{F210090}
Test Plan:
- Vetted missing/surplus/unique messages.
- 146 issues remaining.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10590
Summary: Ref T1191. Nothing too exciting in these.
Test Plan: Saw more blue in UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10521
Summary: See rP8806fb0296c2.
Test Plan:
me fail english
with bonus!
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10514
Summary: ...also re-jiggers all the anchor stuff to use $xaction ID. This seemed like the simplest way once I got in the code, as well as having nice properties for if / when we want to re-add some ajax stuff since the ID is a pretty solid piece of data to key off. Fixes T6083.
Test Plan: mentioned DX in private DX+1. Could see on DX the mention as me and not as the other user. For transactions, I left a comment on Paste and it worked, and I edited an existing transaction and it worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T6083
Differential Revision: https://secure.phabricator.com/D10488
Summary:
Ref T6013. A very long time ago, edges were less clearly low-level infrastructure, and some user-aware stuff got built around edge edits.
This was kind of a mess and I eventually removed it, during or prior to T5245. The big issue was that control flow was really hard to figure out as things went all the way down to the deepest level of infrastructure and then came back up the stack to events and transactions. The new stuff is more top-down and generally seems a lot easier and cleaner.
Consequently, actors are no longer required for edge edits. Remove the parameter.
Test Plan: Poked around; ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T6013
Differential Revision: https://secure.phabricator.com/D10412
Summary:
Ref T6013. Currently, when we create a thumbnail, it gets its own (default) file visibility policy.
In particular, this causes the issue in T6013: thumbnails get "all users" visibility, which does not include logged-out users.
Instead, a thumbnail should just have the same visibility as the original file does. Enforce this:
- When loading thumbnails, reject thumbnails with invisible originals.
- When filtering thumbnails, permit thumbnails with visible originals.
Test Plan: As a logged-out user, thumbnails are now visible when the original files are attached to visible objects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6013
Differential Revision: https://secure.phabricator.com/D10410
Summary:
Primarily, this fixes searching for `F123` in global search.
The info URI is now a better URI than the "best" URI for files, and doesn't have redirect issues.
Test Plan: Searched for `F123` in global search.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10330
Summary:
Fixes T5912. When we write files, we attempt to share storage if two files have the same content.
In some cases, we may not share storage. Examples include:
- Files migrated with `bin/files migrate` (it's simpler not to try to dedupe them).
- Old files, from before storage was sharable (the mechanism did not exist).
- Files broken by the bug fixed in T5912.
Add a script to compact files by pointing files with the same content hash at the same file contnet.
In the particular case of files broken by the bug in T5912, we know the hash of the file's content and will only point them at a file that we can load the data for, so this fixes them.
Compaction is not hugely useful in general, but this script isn't too complex and the ability to fix damage from the bug in T5912 is desirable. We could remove this capability eventually.
Test Plan:
- Ran `files compact --all --dry-run` and sanity checked a bunch of the duplicates for actually being duplicates.
- Migrated individual files with `files compact Fnnn --trace` and verified the storage compacted and all files survived the process.
- Verified unused storage was correctly destroyed after removing the last reference to it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5912
Differential Revision: https://secure.phabricator.com/D10327
Summary:
Fixes T5912. When migrating files, we try to clean up the old data. However, this code isn't aware of reference counting, and unconditionally destroys the old data.
For example, if you migrate files `F1` and `F2` and they have the same data, we'll delete the shared data when we migrate `F1`. Then you'll get an error when you migrate `F2`.
Since this only affects duplicate files, it primarily hits default profile pictures, which are the most numerous duplicate files on most installs.
Test Plan:
- Verified that the theory was correct by uploading two copies of a file and migrating the first one, before applying the patch. The second one's data was nuked and it couldn't be migrated.
- Applied patch.
- Uploaded two copies of a new file, migrated the first one (no data deletion), migrated the second one (data correctly deleted).
- Uploaded two copies of another new file, `bin/remove destory'd` the first one (no data deletion), then did it to the second one (data correctly deleted).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5912
Differential Revision: https://secure.phabricator.com/D10312
Summary: and for bonus, finesse some URIs a tad. Fixes T5922.
Test Plan: viewed F1 logged out and it worked! viewed the ugly URI for F1 and got redirected to the pretty URI.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5922
Differential Revision: https://secure.phabricator.com/D10309
Summary: Ref T5894. We have a couple more similar cases. Make them all do a decision-based redirect for now.
Test Plan: Did "View Raw File" and such, and also made sure thumbnails still work.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5894
Differential Revision: https://secure.phabricator.com/D10301
Summary: Straightforward (this is the one object type we do let you delete from the web UI) implemetation of `PhabricatorDestructibleInterface`.
Test Plan: Used `bin/remove destroy` to destory several files. Used `--trace` to verify they wiped file data.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10300
Summary: Fixes T5894. This needs some improvement when we lay in real CDN stuff, but should get all the cases right for now.
Test Plan: Thumbnails work properly again.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5894
Differential Revision: https://secure.phabricator.com/D10299
Summary: Fixes T5798. We basically weren't using the caching mechanism. Also adds service calls for S3 stuff, and support for seeing a little info like you can for conduit.
Test Plan: uploaded a paste, looked at paste list - no s3 service calls. edited the paste, looked at paste list - no s3 service calls and edited content properly shown
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5798
Differential Revision: https://secure.phabricator.com/D10294
Summary:
Via HackerOne. Chrome (at least) interprets backslashes like forward slashes, so a redirect to "/\evil.com" is the same as a redirect to "//evil.com".
- Reject local URIs with backslashes (we never generate these).
- Fully-qualify all "Location:" redirects.
- Require external redirects to be marked explicitly.
Test Plan:
- Expanded existing test coverage.
- Verified that neither Diffusion nor Phriction can generate URIs with backslashes (they are escaped in Diffusion, and removed by slugging in Phriction).
- Logged in with Facebook (OAuth2 submits a form to the external site, and isn't affected) and Twitter (OAuth1 redirects, and is affected).
- Went through some local redirects (login, save-an-object).
- Verified file still work.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10291
Summary: Ref T5884. We migrated with "canCDN" and then had live writes with "cancdn". Move everything to "canCDN" for consistency.
Test Plan: Ran migration, verified DB only has "canCDN" afterward.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5884
Differential Revision: https://secure.phabricator.com/D10273
Summary:
Ref T5884. We migrated to add a `canCDN` flag, but the code looks for a `cancdn` flag.
If this fixes the issue, I'll migrate `cancdn` to `canCDN` in the next diff.
Test Plan: Viewed some files, including old files, and saw the cacheability I expected.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5884
Differential Revision: https://secure.phabricator.com/D10264
Summary:
Ref T5861. Currently, mail tags are hard-coded; move them into applications. Each Editor defines its own tags.
This has zero impact on the UI or behavior.
Test Plan:
- Checked/unchecked some options, saved form.
- Swapped back to `master` and saw exactly the same values.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10238
Summary:
Fixes T5849. When a new file is created, we might have to actually write the data to a storage engine, or we might be able to just point at data which is already there.
Currently, these two paths handle `$params` with different code and mild behavioral differences. Instead, have them call the same code so they get the same behavior.
Test Plan:
- Uploaded the same file multiple times to home page.
- Uploaded the same file multiple times as profile picture.
- Generated files via Diffusion.
- All the files got the expected properties, whether they were reusing data or not.
Reviewers: btrahan, 20after4
Reviewed By: 20after4
Subscribers: epriestley
Maniphest Tasks: T5849
Differential Revision: https://secure.phabricator.com/D10216
Summary:
Ref T5685. Currently we just 403 on an invalid token, but we can be a little more helpful.
The issues here are:
- If we **do** redirect you on this page and something goes wrong, you might get stuck in a redirect loop.
- If we **don't** redirect you, copy/pasting the link to someone (or reloading the page) gives them a pretty confusing result, since the link doesn't work any more. Prior to this diff, they get a 403.
To mitigate this, do a little better than a bare 403: give them a link to auth and generate a new URI for the file.
If this is still confusing, the next best thing I can come up with is something like this:
- Put some modulous of the timestamp in the URI.
- If the current time is within 2 seconds of the generation time, show this dialog.
- Otherwise, redirect.
That seems like it would be okay, but I worry that "2" has to be small (so links you copy/paste -> chat -> click still work) and a small value means that a small amount of clock skew breaks things. We could use the database clock, but ehhh.
Other ideas:
- Put a hash of the remote IP in the URI, redirect if it doesn't match. Fails for companies behind a NAT gateway but should work in a lot of other cases.
- Just redirect always, there's no reason it should ever loop and browsers don't really do anything bad when there's a loop (they'll show an error after too many redirects).
I'm leaning toward letting this stabilize in the wild for a bit, then trying "always redirect".
Test Plan: {F188914}
Reviewers: btrahan, 20after4
Reviewed By: 20after4
Subscribers: epriestley
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10215
Summary: Ref T5685. We've added a new `canCDN` flag to control whether or not files can be cached and delivered over a CDN. Show this flag in the UI.
Test Plan: Viewed several files, saw correct/expected UI values.
Reviewers: btrahan, 20after4
Reviewed By: 20after4
Subscribers: epriestley
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10213
Summary:
CanCDN flag indicates that a file can be served + cached
via anonymous content distribution networks.
Once D10054 lands, any files that lack the CanCDN flag
will require a one-time-use token and headers will
prohibit cache to protect sensitive files from
unauthorized access.
This diff separates the CanCDN changes from the code that
enforces these restrictions in D10054 so that the changes
can be tested and refined independently.
Test Plan: Work in progress
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: rush898, qgil, epriestley, aklapper, Korvin
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10166
Summary:
This is pretty basic allowing a user to set the
policy as a valid string ('no-one' or 'users') or
as a valid PHID. Without an explicit policy
a permissive one is set.
Test Plan:
Tested using the python-phabricator module (very basic api wrapper).
The arc cli syntax was evading me.
```import base64
from phabricator import Phabricator
phab = Phabricator()
with open('mypic.jpg') as f:
encoded = base64.b64encode(f.read())
//set no-one as viewer which really means author only?
phab.file.upload(name='mypicnoone.jpg',
data_base64=encoded,
viewPolicy='no-one')
//set a specific phid as policy in this case a project
phab.file.upload(name='mypicphid.jpg',
data_base64=encoded,
viewPolicy='PHID-PROJ-fgvvnafmhvkgn2d5a4rf')
//no set policy ends up as 'users' i.e. ('all users')
phab.file.upload(name='mypicdefault.jpg', data_base64=encoded)```
Not able to really test canCDN attribute but it should be
fine and I tried to make it all consistent with D10166
Reviewers: 20after4, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: 20after4, epriestley, Korvin
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10164
Summary:
Ref T4589. When you look at a file, we load attached objects in order to run the "you can see this if you can see any attached object" policy check.
However, right now the subquery inherits the "throw on filter" flag from the parent query. This inheritance makes sense in other cases[1], but because this is an "ANY" rule it does not make sense here. In practice, it means that if the file is attached to several objects, and any of them gets filtered, you can not see the file.
Instead, explicitly drop the flag for this subquery.
[1] Sort of. It doesn't produce wrong results in other cases, but now that I think about it might produce a less-tailored error than it could. I'll look into this the next time I'm poking around.
Test Plan:
- Viewed an "All Users" file attached to a private Mock.
- Prior to this patch, I incorrectly received an exception when the Mock was loaded. This is wrong; I should be able to see the file because the policy is "All Users".
- After the patch, I can correctly view the file, just not the associated mock.
{F127074}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: 20after4, aran, epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D8498
Summary:
Fixes T4589. This implements much better policy behavior for files that aligns with user expectations.
Currently, all files have permissive visibility.
The new behavior is:
- Files uploaded via drag-and-drop to the home page or file upload page get permissive visibility, for ease of quickly sharing things like screenshots.
- Files uploaded via the manual file upload control get permissive visibility by default, but the user can select the policy they want at upload time in an explicit/obvious way.
- Files uploaded via drag-and-drop anywhere else (e.g., comments or Pholio) get restricted visibility (only the uploader).
- When the user applies a transaction to the object which uses the file, we attach the file to the object and punch a hole through the policies: if you can see the object, you can see the file.
- This rule requires things to use ApplicationTransactions, which is why this took so long to fix.
- The "attach stuff to the object" code has been in place for a long time and works correctly.
I'll land D8498 after this lands, too.
Test Plan:
- Uploaded via global homepage upload and file drag-and-drop upload, saw permissive visibility.
- Uploaded via comment area, saw restricted visibility.
- After commenting, verified links were established and the file became visible to users who could see the attached object.
- Verified Pholio (which is a bit of a special case) correctly attaches images.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D10131
Summary: Ref T4589. Depends on D10129. In addition to letting users change the visibility policy for files, also allow them to choose a policy explicitly when a file is uploaded.
Test Plan: Uploaded several files using the plain old uploader, saw appropriate visibility policies applied.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D10130
Summary: Ref T4589. Allow users to adjust visibility settings on files explicitly. This makes it easier to understand and manage upcoming changes in T4589.
Test Plan: Changed the view policy for a file several times.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4589
Differential Revision: https://secure.phabricator.com/D10129
Summary: Ref T5655. Rename `PhabricatorPHIDType` subclasses for clarity (see discussion in D9839). I'm not too keen on some of the resulting class names, so feel free to suggest alternatives.
Test Plan: Ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9986
Summary: Provide an implementation for the `getName` method rather than automagically determining the application name.
Test Plan: Saw reasonable application names in the launcher.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10027
Summary: Ref T5655. Some discussion in D9839. Generally speaking, `Phabricator{$name}Application` is clearer than `PhabricatorApplication{$name}`.
Test Plan:
# Pinned and uninstalled some applications.
# Applied patch and performed migrations.
# Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
# Performed a sanity check on the database contents.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9982
Summary: Instead of implementing the `getTypeConstant` method in all subclasses of `PhabricatorPHIDType`, provide a `final` implementation in the base class which uses reflection. See D9837 for a similar implementation.
Test Plan: Ran `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9985
Summary:
Ref T5245. These were a bad idea.
We no longer need actors for edge edits either, so remove those. Generally, edges have fit into the policy model as pure/low-level infrastructure, and they do not have any policy or capability information in and of themselves.
Test Plan: `grep`
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9840
Summary: These have been moved into libphutil.
Test Plan: Browsed Phabricator, didn't see a crash.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9907
Summary: Fixes T5588. If you upload an image, we currently take you to the image URL, but this makes it hard to figure out the monogram for use elsewhere.
Test Plan: Uploaded a file and was taken to the info page.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5588
Differential Revision: https://secure.phabricator.com/D9872
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary: Remarkup rules can not safely use arbitrary text in tag attributes,
because it may include tokens which are later replaced. Precedence rules
should prevent this in general. Use flat text assertions and adjust precedence
rules in cases where they may not prevent tokens from appearing in attributes.
Auditors: btrahan
Summary: Fixes T5500. When an image is embedded with `{Fxx, size=full}`, add "max-width: 100%;" so that large images are scaled down to the size of the container. This seems like a better and more reasonable behavior than having them scroll. You can still lightbox them or right-click -> view if you really want the full image.
Test Plan: Dragged window around with a very large `size=full` image. At large window sizes, the image displayed at 100%. At smaller window sizes, the image was scaled to fit.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5500
Differential Revision: https://secure.phabricator.com/D9758
Summary:
Fixes T5424.
- One concrete issue: drafts were not being cleared properly because `__draft__` was not set on submission. This (mostly) fixes phantom drafts.
- This ajax comment magic feels weird and floaty and generally has problems. For example, if you add subscribers or inlines, all the stuff on the page which represents those won't update automatically. Instead, just reload. Maybe we'll ajax this stuff some day, but it feels like a net negative for now.
- Also remove it from other applications where it's currently used.
- Fix an issue with inline previews.
Test Plan: Made some comments on a mock, everything worked normally like I expected it to.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5424
Differential Revision: https://secure.phabricator.com/D9649
Summary:
Ref T5359. When users upload non-image file types (PDFs, text files, whatever), Pholio currently chokes in a few places. Make most of these behaviors more reasonable:
- Provide thumbs in the required sizes.
- Predict the thumb size of these files correctly.
- Disable inline comments.
- Make "View Fullsize" and "Download" into buttons. These mostly-work. Download should probaly really download, but CSRF on forms is a bit of a pain right now.
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5359
Differential Revision: https://secure.phabricator.com/D9548
Summary:
This could probably use some refinement (and, like, explanatory text, and stronger cues about what rows and columns mean) but feels fairly good to me, at least on test data.
I didn't do any scrolling for now since we have to do full height on mobile anyway I think. I did swap it so the newer ones are on top.
Left/right navigate you among current images only, but you can click any thumb to review history.
Removed history view since it's no longer useful.
Some things that would probably help:
- Some kind of header explaining what this is ("Mock History" or something).
- Stronger visual cue that columns are related by being the same image.
- Clearer cues about obsolete/deleted images (e.g., on the stage itself?)
- Maybe general tweaks.
- Maybe a placeholder (like a grey "X") for images which have been deleted.
(I'm planning to add comment counts too, which I think will be pretty useful, but that felt good to put in another diff.)
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9543
Summary: Ref T4986. Instead of requiring users to know the name of an application search engine class, let them select from a list.
Test Plan:
Created a new panel.
{F165468}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9500
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9431
Summary: Did a more exhaustive grep on setIcon and found 99.9% of the icons.
Test Plan: I verified icon names on UIExamples, but unable to test some of the more complex flows visually. Mostly a read and replace.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9088
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.
Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9052
Summary: Ref T4986. These are mostly mechanical now, I skipped a couple of slightly tricky ones. Still a bunch to go.
Test Plan:
For each engine:
- Viewed the application;
- created a panel to issue the query.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9017
Summary:
This is just a general review of config options, to reduce the amount of damage a rogue administrator (without host access) can do. In particular:
- Fix some typos.
- Lock down some options which would potentially let a rogue administrator do something sketchy.
- Most of the new locks relate to having them register a new service account, then redirect services to their account. This potentially allows them to read email.
- Lock down some general disk stuff, which could be troublesome in combination with other vulnerabilities.
Test Plan:
- Read through config options.
- Tried to think about how to do evil things with each one.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8928
Summary:
Ref T4843.
- Add an `alt` attribute so users can provide alternate text for `{Fnnn}`.
- Add an `alt` attribute to image macros.
Test Plan: Embedded an image with `alt` and a macro, inspected HTML source to verify the `alt` attribute was present.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4843
Differential Revision: https://secure.phabricator.com/D8925
Summary: Added .phabricator-remarkup-embed-image to full size images as well
Test Plan: Add an image e.g. `{F123, size=full}` and verify that it has a shadow and the space next to it isn't clickable
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4902
Differential Revision: https://secure.phabricator.com/D8858
Summary: A small but appreciable number of users find flavor on buttons confusing. Remove this flavor. This retains flavor in headers, error messages, etc., which doesn't cause confusion.
Test Plan: Looked at a revision, task, paste, macro, etc.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8812
Summary:
- Point them at the new Diviner.
- Make them a little less cumbersome to write.
Test Plan: Found almost all of these links in the UI and clicked them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8553
Summary:
Ref T2479, T4406. We should do a better job of (a) handling image processing errors and (b) declining to process large image files.
This fixes the worst of it, which is that users can upload huge GIFs with a large number of frames and hang a `convert` process for a long time, eating a CPU and a pile of memory.
This code is still pretty iffy and needs some more work. A near-term product goal for it is supporting 100x100 profile images.
Test Plan: Uploaded large and small GIFs, after setting the definition of "enormous" to be pretty small. Saw the small GIFs thumbnail into animated GIFs, and the large ones thumbnail into static images.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2479, T4406
Differential Revision: https://secure.phabricator.com/D8536
Summary:
Via HackerOne. In regular expressions, "$" matches "end of input, or before terminating newline". This means that the expression `/^A$/` matches two strings: `"A"`, and `"A\n"`.
When we care about this, use `\z` instead, which matches "end of input" only.
This allowed registration of `"username\n"` and similar.
Test Plan:
- Grepped codebase for all calls to `preg_match()` / `preg_match_all()`.
- Fixed the ones where this seemed like it could have an impact.
- Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Differential Revision: https://secure.phabricator.com/D8516
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.
This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.
Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8460
