Summary:
Ref T13603. On common edit pathways, extract explicit file attachments from Remarkup. These pathways are affected:
- Objects that use EditEngine and expose a remarkup area via "RemarkupEditField".
- Objects that use EditEngine to generate a comment area.
This is the vast majority of pathways, but not entirely exhaustive.
Test Plan: Created and commented on a task, explicitly attaching images. Saw images attach properly.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21830
Summary:
Fixes T12919. Fixes T13636. Prior to this change, some well-known resource paths don't route on sites like ResourceSite.
- `/robots.txt`: Make it route on ResourceSite and just deny the whole site.
- `/favicon.ico`: Make it route on ResourceSite.
- `/status/`: Make it route on ResourceSite.
- 404: Make it render a 404 on ResourceSite.
Test Plan:
- Visited all URIs on ResourceSite, got sensible responses.
- Visited all URIs on main site.
- Visited 404 while logged out, got login page.
Maniphest Tasks: T13636, T12919
Differential Revision: https://secure.phabricator.com/D21606
Summary:
Ref T13602. Currently, Hovercards are functions only of the object they represent (and the viewer, etc).
Recent changes to how users who can't see an object are rendered motivate making them a function of both the object they represent //and// the context in which they are being viewed. In particular, this enables a hovecard for a user to explain "This user can't see the thing you're lookign at right now.", so visual "exiled" markers can have a path forward toward discovery.
Test Plan:
- This change isn't expected to affect any behavior.
- Viewed hovercards, moused over/out, resized windows, viewed standalone cards, viewed debug cards, saw no behavioral changes.
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21553
Summary:
Ref T13575. Since PHP builtin webserver support was added, the pathway for parsing request parameters became more complex. We now rebuild "$_REQUEST" later, and this rebuild will destroy any mutations made to it here, so the assignment to "__path__" is lost.
Instead of "validating" the request path, make this method "read" the request path and store it explicitly, so it will survive any later request mutations.
Test Plan:
- Submitted any POST form while running Phabricator under the builtin PHP webserver. Old behavior was an error when accessing "__path__"; new behavior is a working application.
- Loaded normal pages, etc.
Maniphest Tasks: T13575
Differential Revision: https://secure.phabricator.com/D21506
Summary:
Ref T13517. See that task for details about the underlying issue here.
Currently, we may decode a compressed response, then retransmit it with leftover "Content-Encoding" and "Content-Length" headers. Instead, strip these headers.
Test Plan:
- In a clustered repository setup, cloned a Git repository over HTTP.
- Before: Error while processing content unencoding: invalid stored block lengths
- After: Clean clone.
Maniphest Tasks: T13517
Differential Revision: https://secure.phabricator.com/D21167
Summary:
Ref T4369. During T13507, I set my "max_post_size" to a very small value, like 7 (i.e., 7 bytes). This essentially disables "enable_post_data_reading" even if the setting is technically on.
This breaks forms which use "multipart/form-data", which are rare but not nonexistent. Notably, forms in Config use this setting (because of `ui.header` stuff?) although perhaps they should not or no longer need to.
This can be fixed by parsing the raw input.
Since the only reason we don't parse the raw input is concern that we may not be able to read it (per documentation, but never actually observed), and we do a `strlen()` test anyway, just read it unconditionally.
This should fix cases where POST data wasn't read because of "max_post_size" without impacting anything else.
Test Plan: With very small "max_post_size", updated "ui.footer-items" in Config. Before: form acted as a no-op. After: form submitted.
Maniphest Tasks: T4369
Differential Revision: https://secure.phabricator.com/D21165
Summary:
Ref T13515. Settings currently has some highly specialized code for rendering "Changes saved." messages. The "saved" state is communicated across a redirect-after-POST by adding `/saved/` to the end of the URI.
This isn't great. It needs a lot of moving pieces, including special accommodations in routing rules. It's user-visible. It has the wrong behavior if you reload the page or navigate directly to the "saved" URI.
Try this scheme, which is also pretty sketchy but seems like an upgrade on the balance:
- Set a cookie on the redirect which identifies the form we just saved.
- On page startup: if this cookie exists, save the value and clear it.
- If the current page started with a cookie identifying the form on the page, treat the page as a "saved" page.
This supports passing a small amount of state across the redirect-after-POST flow, and when you reload the page it doesn't keep the message around. Applications don't need to coordinate it, either. Seems somewhat cleaner?
Test Plan: In Firefox, Safari, and Chrome: saved settings, saw a "Saved changes" banner without any URI junk. Reloaded page, saw banner vanish properly.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21144
Summary: Ref T13507. If we believe the server can accept "Content-Encoding: gzip" requests, make the claim in an "X-Conduit-Capabilities" header in responses. Clients can use request compression on subsequent requests.
Test Plan: See D21119 for the client piece.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21120
Summary: Ref T13507. See that task for discussion.
Test Plan: Faked different response behaviors and hit both variations of this error.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21116
Summary:
We write some synthetic HTTP responses inside unit tests. Some responses have an indirect side effect of adjusting "zlib.output_compression", but this adjustment fails if headers have already been output. From a CLI context, headers appear to count as already-output after we write anything to stdout:
```
<?php
echo headers_sent() ? "Y" : "N";
echo "\n";
echo headers_sent() ? "Y" : "N";
echo "\n";
```
This script prints "N", then "Y".
Recently, the default severity of warnings was increased in libphutil; this has been a long-standing warning but now causes test failures.
This behavior is sort of silly but the whole thing is kind of moot anyway. Just skip it if "headers_sent()" is true.
Test Plan: Ran "arc unit --everything", got clean results.
Differential Revision: https://secure.phabricator.com/D21055
Summary: Ref T13395. Move cache classes, syntax highlighters, other markup classes, and sprite sheets to Phabricator.
Test Plan: Attempted to find any callers for any of this stuff in libphutil or Arcanist and couldn't.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20977
Summary: Ref T13395. Moves some Aphront classes from libphutil to Phabricator.
Test Plan: Grepped for symbols in libphutil and Arcanist.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20975
Summary: This text is significantly more clear and helpful for users.
Test Plan: Tried to do something I'm not suppposed to, hit the 403 page.
Differential Revision: https://secure.phabricator.com/D20847
Summary: Refresh the 404 text since it hasn't been updated in a while, and swap the "Save Query" button back to grey since I never got used to blue.
Test Plan: Hit 404 page, saved a query.
Differential Revision: https://secure.phabricator.com/D20734
Summary:
Depends on D20719. Currently, if a page throws an exception (like a policy exception) and rendering that exception into a response (like a policy dialog) throws another exception (for example, while constructing breadcrumbs), we only show the orginal exception.
This is usually the more useful exception, but sometimes we actually care about the other exception.
Instead of guessing which one is more likely to be useful, throw them both as an "AggregateException" and let the high-level handler flatten it for display.
Test Plan: {F6749312}
Differential Revision: https://secure.phabricator.com/D20720
Summary:
Ref T13289. If you do this:
- Subscribe to a task (so we don't generate a subscribe side-effect later).
- Prepare a transaction group: sign with MFA, change projects (don't make any changes), add a comment.
- Submit the transaction group.
...you'll get prompted "Some actions don't have any effect (the non-change to projects), apply remaining effects?".
If you confirm, you get MFA'd, but the MFA flow loses the "continue" confirmation, so you get trapped in a workflow loop of confirming and MFA'ing.
Instead, retain the "continue" bit through the MFA.
Also, don't show "You can't sign an empty transaction group" if there's a comment.
See also T13295, since the amount of magic here can probably be reduced. There's likely little reason for "continue" or "hisec" to be magic nowadays.
Test Plan:
- Went through the workflow above.
- Before: looping workflow.
- After: "Continue" carries through the MFA gate.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20552
Summary: Depends on D20474. Ref T13272. Provide an easy way to rearrange tabs on a tab panel, by moving them left or right from the context menu.
Test Plan: Moved tabs left and right. Tried to move them off the end of the tab list, no luck.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20475
Summary:
Ref T13259. In some configurations, making a request to ourselves may return a VPN/Auth response from some LB/appliance layer.
If this response begins or ends with whitespace, we currently detect it as "extra whitespace" instead of "bad response".
Instead, require that the response be nearly correct (valid JSON with some extra whitespace, instead of literally anything with some extra whitespace) to hit this specialized check. If we don't hit the specialized case, use the generic "mangled" response error, which prints the actual body so you can figure out that it's just your LB/auth thing doing what it's supposed to do.
Test Plan:
- Rigged responses to add extra whitespace, got "Extra Whitespace" (same as before).
- Rigged responses to add extra non-whitespace, got "Mangled Junk" (same as before).
- Rigged responses to add extra whitespace and extra non-whitespace, got "Mangled Junk" with a sample of the document body instead of "Extra Whitespace" (improvement).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20235
Summary:
Fixes T13260. "QUERY_STRING" and "REQUEST_URI" are similar for our purposes here, but our nginx documentation tells you to pass "QUERY_STRING" and doesn't tell you to pass "REQUEST_URI". We also use "QUERY_STRING" in a couple of other places already, and already have a setup check for it.
Use "QUERY_STRING" instead of "REQUEST_URI".
Test Plan: Visited `/oauth/google/?a=b`, got redirected with parameters preserved.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13260
Differential Revision: https://secure.phabricator.com/D20227
Summary:
In D20100, I changed this page from returning a `newPage()` with a dialog as its content to returning a more modern `newDialog()`.
However, the magic to add stuff to the CSP header is actually only on the `newPage()` pathway today, so this accidentally dropped the extra "Content-Security-Policy" rule for Google.
Lift the magic up one level so both Dialog and Page responses hit it.
Test Plan:
- Configured Recaptcha.
- Between D20100 and this patch: got a CSP error on the Email Login page.
- After this patch: clicked all the pictures of cars / store fronts.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20163
Summary: Ref T13250. See D20149. Mostly: clarify semantics. Partly: remove magic "null" behavior.
Test Plan: Poked around, but mostly just inspection since these are pretty much one-for-one.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20154
Summary:
Ref T13250. A handful of callsites are doing `getRequestURI()` + `setQueryParams(array())` to get a bare request path.
They can just use `getPath()` instead.
Test Plan: See inlines.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20150
Summary:
Ref T13250. See PHI1069. This is a small fix for `getRequestURI()` currently not working if the request includes "x[]=..." PHP-flavored array parameters, beacause they're parsed into arrays by `$_GET` and `setQueryParams(...)` no longer accepts nonscalars.
Instead, just parse the raw request URI.
Test Plan: Visited `/search/hovercard/?phids[]=X`, no more fatal. Dumped the resulting URI, saw it had the right value. Tried `?phids[]=x&x=1&x=1&x=1`, saw the parameters correctly preserved.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20147
Summary:
Depends on D20140. Ref T13250. Currently, the top-level exception handler doesn't dump stacks because we might not be in debug mode, and we might double-extra-super fatal if we call `PhabricatorEnv:...` to try to figure out if we're in debug mode or not.
We can get around this by setting a flag on the Sink once we're able to confirm that we're in debug mode. Then it's okay for the top-level error handler to show traces.
There's still some small possibility that showing a trace could make us double-super-fatal since we have to call a little more code, but AphrontStackTraceView is pretty conservative about what it does and 99% of the time this is a huge improvement.
Test Plan: {F6205122}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20142
Summary:
Depends on D20137. Ref T13250. Ref T12101. In versions of PHP beyond 7, various engine errors are gradually changing from internal fatals or internal errors to `Throwables`, a superclass of `Exception`.
This is generally a good change, but code written against PHP 5.x before `Throwable` was introduced may not catch these errors, even when the code is intended to be a top-level exception handler.
(The double-catch pattern here and elsewhere is because `Throwable` does not exist in older PHP, so `catch (Throwable $ex)` catches nothing. The `Exception $ex` clause catches everything in old PHP, the `Throwable $ex` clause catches everything in newer PHP.)
Generalize some `Exception` into `Throwable`.
Test Plan:
- Added a bogus function call to the rendering stack.
- Before change: got a blank page.
- After change: nice exception page.
{F6205012}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250, T12101
Differential Revision: https://secure.phabricator.com/D20138
Summary:
Ref T13250. When exceptions occur in display/rendering/writing, they currently go straight to the fallback handler. This is a minimal handler which doesn't show a stack trace or include any debugging details.
In some cases, we have to do this: some of these exceptions prevent us from building a normal page. For example, if the menu bar has a hard fatal in it, we aren't going to be able to build a nice exception page with a menu bar no matter how hard we try.
However, in many cases the error is mundane: something detected something invalid and raised an exception during rendering. In these cases there's no problem with the page chrome or the rendering pathway itself, just with rendering the page data.
When we get a rendering/response exception, try a second time to build a nice normal exception page. This will often work. If it doesn't work, fall back as before.
Test Plan:
- Forced the error from T13250 by applying D20136 but not D20134.
- Before:
{F6205001}
- After:
{F6205002}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20137
Summary:
On instances, the "SiteSource" (for site config) pretty much copy-pastes the "read POST data" block because it needs to make some decisions based on POST data when handling inbound mail webhooks.
Move the upstream read a little earlier so we can get rid of this. Now that this step is separated and must happen before the profiler, there's no reason not to do it earlier.
Test Plan: POSTed some data across pages without issue, will remove duplicate code in upcoming change.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20073
Summary: Now that we have a nice function for this, use it to simplify some code.
Test Plan: Ran through the Duo enroll workflow to make sure signing still works.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20053
Summary:
Ref T4369. Ref T12297. Ref T13242. Ref PHI1010. I want to take a quick look at `transaction.search` and see if there's anything quick and obvious we can do to improve performance.
On `secure`, the `__profile__` flag does not survive POST like it's supposed to: when you profile a page and then submit a form on the page, the result is supposed to be profiled. The intent is to make it easier to profile Conduit calls.
I believe this is because we're hooking the profiler, then rebuilding POST data a little later -- so `$_POST['__profile__']` isn't set yet when the profiler checks.
Move the POST rebuild a little earlier to fix this.
Also, remove the very ancient "aphront.default-application-configuration-class". I believe this was only used by Facebook to do CIDR checks against corpnet or something like that. It is poorly named and long-obsolete now, and `AphrontSite` does everything we might reasonably have wanted it to do.
Test Plan: Poked around locally without any issues. Will check if this fixes the issue on `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242, T12297, T4369
Differential Revision: https://secure.phabricator.com/D20046
Summary: Depends on D20057. Currently, we show an "MFA" message on one of these and an "Error" message on the other, with different icons and colors. Use "MFA" for both, with the MFA icon / color.
Test Plan: Hit both varations, saw more consistency.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20059
Summary:
Depends on D20026. Ref T13222. Ref T13231. The primary change here is that we'll no longer send you an SMS if you hit an MFA gate without CSRF tokens.
Then there's a lot of support for genralizing into Duo (and other push factors, potentially), I'll annotate things inline.
Test Plan: Implemented Duo, elsewhere.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13231, T13222
Differential Revision: https://secure.phabricator.com/D20028
Summary:
Depends on D19899. Ref T13222. When we prompt you for one-shot MFA, we currently give you a lot of misleading text about your session staying in "high security mode".
Differentiate between one-shot and session upgrade MFA, and give the user appropriate cues and explanatory text.
Test Plan:
- Hit one-shot MFA on an "mfa" task in Maniphest.
- Hit session upgrade MFA in Settings > Multi-Factor.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19900
Summary:
Ref T13222. See PHI873. Ref T9770.
Currently, we support only TOTP MFA. For some MFA (SMS and "push-to-app"-style MFA) we may need to keep track of MFA details (e.g., the code we SMS'd you). There isn't much support for that yet.
We also currently allow free reuse of TOTP responses across sessions and workflows. This hypothetically enables some "spyglass" attacks where you look at someone's phone and type the code in before they do. T9770 discusses this in more detail, but is focused on an attack window starting when the user submits the form. I claim the attack window opens when the TOTP code is shown on their phone, and the window between the code being shown and being submitted is //much// more interesting than the window after it is submitted.
To address both of these cases, start tracking MFA "Challenges". These are basically a record that we asked you to give us MFA credentials.
For TOTP, the challenge binds a particular timestep to a given session, so an attacker can't look at your phone and type the code into their browser before (or after) you do -- they have a different session. For now, this means that codes are reusable in the same session, but that will be refined in the future.
For SMS / push, the "Challenge" would store the code we sent you so we could validate it.
This is mostly a step on the way toward one-shot MFA, ad-hoc MFA in comment action stacks, and figuring out what's going on with Duo.
Test Plan:
- Passed MFA normally.
- Passed MFA normally, simultaneously, as two different users.
- With two different sessions for the same user:
- Opened MFA in A, opened MFA in B. B got a "wait".
- Submitted MFA in A.
- Clicked "Wait" a bunch in B.
- Submitted MFA in B when prompted.
- Passed MFA normally, then passed MFA normally again with the same code in the same session. (This change does not prevent code reuse.)
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222, T9770
Differential Revision: https://secure.phabricator.com/D19886
Summary:
Ref T13216. See PHI980. Currently, each application in {nav Applications > X > Configure} has a "Can Configure Application" permission which is hard-coded to "Administrators".
There's no technical reason for this, there just hasn't been a great use case for unlocking it. I think when I originally wrote it our protections against locking yourself out of things weren't that great (i.e., it was easier to set the policy to something that prevented you from editing it after the new policy took effect). Our protections are better now.
The major goal here is to let installs open up Custom Forms for given applications (mostly Maniphest) to more users, but the other options mostly go hand-in-hand with that.
Also, in developer mode, include stack traces for policy exceptions. This makes debugging weird stuff (like the indirect Config application errors here) easier.
Test Plan:
- Granted "Can Configure Application" for Maniphest to all users.
- Edited custom forms as a non-administrator.
- Configured Maniphest as a non-administrator.
- Installed/uninstalled Maniphest as a non-administrator.
- Tried to lock myself out (got an error message).
{F6015721}
Reviewers: amckinley, joshuaspence
Reviewed By: joshuaspence
Subscribers: joshuaspence
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19822
Summary:
Ref T12907. At least part of the problem is that we can hit PHP's `max_execution_time` limit on the file download pathway.
We don't currently set this to anything in the application itself, but PHP often sets it to 30s by default (and we have it set to 30s in production).
When writing responses, remove this limit. This limit is mostly a protection against accidental loops/recurison/etc., not a process slot protection. It doesn't really protect process slots anyway, since it doesn't start counting until the request starts executing, so you can (by default) //send// the request as slowly as you want without hitting this limit.
By releasing the limit this late, hopefully all the loops and recursion issues have already been caught and we're left with mostly smooth sailing.
We already remove this limit when sending `git clone` responses in `DiffusionServeController` and nothing has blown up. This affects `git clone http://` and similar.
(I may have had this turned off locally and/or just be too impatient to wait 30s, which is why I haven't caught this previously.)
Test Plan:
- Poked around and downloaded some files.
- Will `curl ...` in production and see if that goes better.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12907
Differential Revision: https://secure.phabricator.com/D19547
Summary:
See D19394. Currently, during first-time setup before you configure "phabricator.base-uri", we may attempt to generate a setup page, try to generate a CSP header for it, and fail to access the environmental config. This causes a too-severe error page ("configure phabricator.base-uri") instead of preflight guidance (like "can't connect to MySQL").
Instead, treat this more like "security.alternate-file-domain" and just bail on CSP if we can't fetch it.
Test Plan: On a fresh (non-explodey laptop) install with critical setup errors (no MySQL installed yet), loaded Phabricator. Before: error about phabricator.base-uri. After: more helpful guidance about installing/configuring MySQL.
Reviewers: amckinley, avivey
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19396
Summary:
Ref T13105. Fixes some issues with line linking and highlighting under DocumentEngine:
- Adding `$1-3` to the URI didn't work correctly with query parameters.
- Reading `$1-3` from the URI didn't work correctly because Diffusion parses them slightly abnormally.
Test Plan: Clicked/dragged lines to select them. Observed URI. Reloaded page, got the right selection.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19305
Summary:
Depends on D19251. Ref T13105. This adds rendering engine support for PDFs.
It doesn't actually render them, it just renders a link which you can click to view them in a new window. This is much easier than actually rendering them inline and at least 95% as good most of the time (and probably more-than-100%-as-good some of the time).
This makes PDF a viewable MIME type by default and adds a narrow CSP exception for it. See also T13112.
Test Plan:
- Viewed PDFs in Files, got a link to view them in a new tab.
- Clicked the link in Safari, Chrome, and Firefox; got inline PDFs.
- Verified primary CSP is still `object-src 'none'` with `curl ...`.
- Interacted with the vanilla lightbox element to check that it still works.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19252
Summary:
Ref T4340. See <https://discourse.phabricator-community.org/t/core-exception-during-installation/1193/8>.
If we return a response very early during setup, we may not be able to read from the environment yet. Just decline to build a "Content-Security-Policy" header in these cases.
Test Plan:
- Faked a preflight error (e.g., safe_mode enabled), restarted apache.
- Before patch: environment error while generating CSP.
- After patch: no error.
- Loaded a normal page, observed an normal CSP header.
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D19172
Summary:
Ref T4340. If you don't configure a CDN and visit a custom site (like a Phame blog site, or a CORGI sandbox internally) we serve resources from the main site. This violates the Content-Security-Policy.
When there's no CDN, include the primary domain in the CSP explicitly.
Test Plan: Loaded `local.www.phacility.com`, got resources.
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D19170
Summary: Ref T13088. This lifts the code for parsing "$x-y" line ranges in URIs into AphrontRequest so Diffusion, Paste, Harbormaster, etc., can share it.
Test Plan: Viewed lines, line ranges, no lines, negative line ranges, line ranges with 0, and extremely long line ranges in Paste.
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19162
Summary:
Ref T4340. Some "Register/Login" and "Link External Account" buttons are forms which submit to third-party sites. Whitelist these targets when pages render an OAuth form.
Safari, at least, also prevents a redirect to a third-party domain after a form submission to the local domain, so when we first redirect locally (as with Twitter and other OAuth1 providers) we need to authorize an additional URI.
Test Plan: Clicked all my registration buttons locally without hitting CSP issues.
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D19159
Summary: Ref T4340. We don't use "<base />" so we can safely block it.
Test Plan: Injected "<base />" into a page, saw an error in the console showing that the browser had blocked it.
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D19158
Summary:
Depends on D19155. Ref T13094. Ref T4340.
We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `<form />` which sometimes POSTs to the CDN domain.
Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better.
Then, implement the stricter Content-Security-Policy.
This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports.
Test Plan:
- Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain.
- Went through all those workflows again without a CDN domain.
- Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`).
- Added an evil form to a page, tried to submit it, was rejected.
- Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13094, T4340
Differential Revision: https://secure.phabricator.com/D19156
Summary: See PHI399. Ref T4340. We don't require Flash/Java anywhere and can safely block them unconditionally in the Content-Security-Policy header.
Test Plan: Added a `<object ... />` tag to a page, saw "Blocked Plug-In" and a CSP warning in the browser console.
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D19154
Summary:
Ref T4340. Some browsers respect this header and referrers are a plague upon the earth.
Also, upgrade "never" to the more modern value "no-referrer".
Test Plan:
In Safari, Firefox and Chrome, disabled `rel="noreferrer"` on links and generated a normal link to an external site. Then clicked it and checked if a referrer was sent.
- Safari respects meta only, but "no-referrer" is fine.
- Firefox respects both (either the header or meta tag are individually sufficient to stop referrers).
- Chrome respects both (same as Firefox).
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D19144
