revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-21 22:32:41 +01:00
Code Issues Releases Wiki Activity

Remove an old digest in Celerity code and some obsolete configuration options

Browse source 
Summary:
Ref T12509. This upgrades a `weakDigest()` callsite to SHA256-HMAC and removes three config options:

  - `celerity.resource-hash`: Now hard-coded, since the use case for ever adjusting it was very weak.
  - `celerity.enable-deflate`: Intended to make cache inspection easier, but we haven't needed to inspect caches in ~forever.
  - `celerity.minify`: Intended to make debugging minification easier, but we haven't needed to debug this in ~forever.

In the latter two cases, the options were purely developer-focused, and it's easy to go add an `&& false` somewhere in the code if we need to disable these features to debug something, but the relevant parts of the code basically work properly and never need debugging. These options were excessively paranoid, based on the static resource enviroment at Facebook being far more perilous.

The first case theoretically had end-user utility for fixing stuck content caches. In modern Phabricator, it's not intuitive that you'd go adjust a Config option to fix this. I don't recall any users ever actually running into problems here, though.

(An earlier version of this change did more magic with `celerity.resource-hash`, but this ended up with a more substantial simplification.)

Test Plan: Grepped for removed configuration options.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D19941
This commit is contained in:
epriestley 2019-01-01 20:28:55 -08:00
parent 7e87d254ab
commit afa69eedd1
9 changed files with 1939 additions and 1972 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
conf/__init_conf__.php
View file

@ -21,7 +21,6 @@ function phabricator_read_config_file($original_config) {
return array(
'phabricator.developer-mode' => true,
'darkconsole.enabled' => true,
'celerity.minify' => false,
);
}

3822
resources/celerity/map.php
View file

File diff suppressed because it is too large Load diff

3
src/applications/cache/PhabricatorKeyValueDatabaseCache.php vendored
View file

@ -145,8 +145,7 @@ final class PhabricatorKeyValueDatabaseCache
static $can_deflate;
if ($can_deflate === null) {
$can_deflate = function_exists('gzdeflate') &&
PhabricatorEnv::getEnvConfig('cache.enable-deflate');
$can_deflate = function_exists('gzdeflate');
}
if ($can_deflate) {

18
src/applications/celerity/CelerityResourceMapGenerator.php
View file

@ -156,7 +156,7 @@ EOFILE;
$result_map = array();
foreach ($binary_map as $name => $data_hash) {
$hash = $resources->getCelerityHash($data_hash.$name);
$hash = $this->newResourceHash($data_hash.$name);
$result_map[$name] = array(
'hash' => $hash,
@ -185,8 +185,8 @@ EOFILE;
$raw_data = $resources->getResourceData($name);
$xformed_data = $xformer->transformResource($name, $raw_data);
$data_hash = $resources->getCelerityHash($xformed_data);
$hash = $resources->getCelerityHash($data_hash.$name);
$data_hash = $this->newResourceHash($xformed_data);
$hash = $this->newResourceHash($data_hash.$name);
list($provides, $requires) = $this->getProvidesAndRequires(
$name,
@ -324,7 +324,7 @@ EOFILE;
$hashes[] = $symbol.':'.$symbol_hash;
}
$hash = $resources->getCelerityHash(implode("\n", $hashes));
$hash = $this->newResourceHash(implode("\n", $hashes));
$package_map[$package_name] = array(
'hash' => $hash,
'symbols' => $package_symbols,
@ -394,4 +394,14 @@ EOFILE;
return $list;
}
private function newResourceHash($data) {
// This HMAC key is a static, hard-coded value because we don't want the
// hashes in the map to depend on database state: when two different
// developers regenerate the map, they should end up with the same output.
$hash = PhabricatorHash::digestHMACSHA256($data, 'celerity-resource-data');
return substr($hash, 0, 8);
}
}

4
src/applications/celerity/controller/CelerityPhabricatorResourceController.php
View file

@ -39,10 +39,8 @@ final class CelerityPhabricatorResourceController
}
protected function buildResourceTransformer() {
$minify_on = PhabricatorEnv::getEnvConfig('celerity.minify');
$developer_on = PhabricatorEnv::getEnvConfig('phabricator.developer-mode');
$should_minify = ($minify_on && !$developer_on);
$should_minify = !$developer_on;
return id(new CelerityResourceTransformer())
->setMinify($should_minify)

6
src/applications/celerity/resources/CelerityResources.php
View file

@ -12,12 +12,6 @@ abstract class CelerityResources extends Phobject {
return 0;
}
public function getCelerityHash($data) {
$tail = PhabricatorEnv::getEnvConfig('celerity.resource-hash');
$hash = PhabricatorHash::weakDigest($data, $tail);
return substr($hash, 0, 8);
}
public function getResourceType($path) {
return CelerityResourceTransformer::getResourceType($path);
}

8
src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
View file

@ -372,6 +372,14 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
'phpmailer.smtp-encoding' => $mailers_reason,
'sendgrid.api-user' => $mailers_reason,
'sendgrid.api-key' => $mailers_reason,
'celerity.resource-hash' => pht(
'This option generally did not prove useful. Resource hash keys '.
'are now managed automatically.'),
'celerity.enable-deflate' => pht(
'Resource deflation is now managed automatically.'),
'celerity.minify' => pht(
'Resource minification is now managed automatically.'),
);
return $ancient_config;

34
src/applications/config/option/PhabricatorDeveloperConfigOptions.php
View file

@ -146,38 +146,12 @@ final class PhabricatorDeveloperConfigOptions
pht('Enable developer mode'),
pht('Disable developer mode'),
))
->setSummary(pht('Enable verbose error reporting and disk reads.'))
->setDescription(
pht(
'This option enables verbose error reporting (stack traces, '.
'error callouts) and forces disk reads of static assets on '.
'every reload.')),
$this->newOption('celerity.minify', 'bool', true)
->setBoolOptions(
array(
pht('Minify static resources.'),
pht("Don't minify static resources."),
))
->setSummary(pht('Minify static Celerity resources.'))
->setSummary(pht('Enable verbose error reporting and disk reads.'))
->setDescription(
pht(
'Minify static resources by removing whitespace and comments. You '.
'should enable this in production, but disable it in '.
'development.')),
$this->newOption('cache.enable-deflate', 'bool', true)
->setBoolOptions(
array(
pht('Enable deflate compression'),
pht('Disable deflate compression'),
))
->setSummary(
pht('Toggle %s-based compression for some caches.', 'gzdeflate()'))
->setDescription(
pht(
'Set this to false to disable the use of %s-based '.
'compression in some caches. This may give you less performant '.
'(but more debuggable) caching.',
'gzdeflate()')),
'This option enables verbose error reporting (stack traces, '.
'error callouts) and forces disk reads of static assets on '.
'every reload.')),
);
}
}

15
src/applications/config/option/PhabricatorSecurityConfigOptions.php
View file

@ -234,21 +234,6 @@ EOTEXT
'Users can configure a URI pattern to open files in a text '.
'editor. The URI must use a protocol on this whitelist.'))
->setLocked(true),
$this->newOption(
'celerity.resource-hash',
'string',
'd9455ea150622ee044f7931dabfa52aa')
->setSummary(
pht('An input to the hash function when building resource hashes.'))
->setDescription(
pht(
'This value is an input to the hash function when building '.
'resource hashes. It has no security value, but if you '.
'accidentally poison user caches (by pushing a bad patch or '.
'having something go wrong with a CDN, e.g.) you can change this '.
'to something else and rebuild the Celerity map to break user '.
'caches. Unless you are doing Celerity development, it is '.
'exceptionally unlikely that you need to modify this.')),
$this->newOption('remarkup.enable-embedded-youtube', 'bool', false)
->setBoolOptions(
array(