From 6829003f5e45fc1493d3e5022c145515aad7e0a4 Mon Sep 17 00:00:00 2001 From: James Cole Date: Sat, 11 Apr 2020 06:42:21 +0200 Subject: [PATCH 1/9] Change to safer hash methods. --- app/Handlers/Events/UserEventHandler.php | 3 ++- app/Http/Controllers/JavascriptController.php | 2 +- app/Http/Controllers/ProfileController.php | 2 +- app/Repositories/ImportJob/ImportJobRepository.php | 2 +- app/Services/Password/PwndVerifierV2.php | 9 ++++++--- app/Support/CacheProperties.php | 4 ++-- app/Support/Preferences.php | 3 +-- .../migrations/2016_06_16_000002_create_main_tables.php | 2 +- 8 files changed, 15 insertions(+), 12 deletions(-) diff --git a/app/Handlers/Events/UserEventHandler.php b/app/Handlers/Events/UserEventHandler.php index 91964dd90d..964d0b11e1 100644 --- a/app/Handlers/Events/UserEventHandler.php +++ b/app/Handlers/Events/UserEventHandler.php @@ -165,7 +165,8 @@ class UserEventHandler $user = $event->user; $ipAddress = $event->ipAddress; $token = app('preferences')->getForUser($user, 'email_change_undo_token', 'invalid'); - $uri = route('profile.undo-email-change', [$token->data, hash('sha256', $oldEmail)]); + $hashed = hash('sha256', sprintf('%s%s', (string) config('app.key'), $oldEmail)); + $uri = route('profile.undo-email-change', [$token->data,$hashed]); try { Mail::to($oldEmail)->send(new UndoEmailChangeMail($newEmail, $oldEmail, $uri, $ipAddress)); // @codeCoverageIgnoreStart diff --git a/app/Http/Controllers/JavascriptController.php b/app/Http/Controllers/JavascriptController.php index 240c78a98d..5bd6bc9874 100644 --- a/app/Http/Controllers/JavascriptController.php +++ b/app/Http/Controllers/JavascriptController.php @@ -126,7 +126,7 @@ class JavascriptController extends Controller /** @noinspection NullPointerExceptionInspection */ $lang = $pref->data; $dateRange = $this->getDateRangeConfig(); - $uid = substr(hash('sha256', auth()->user()->id . auth()->user()->email), 0, 12); + $uid = substr(hash('sha256', sprintf('%s-%s-%s', (string) config('app.key'), auth()->user()->id, auth()->user()->email)), 0, 12); $data = [ 'currencyCode' => $currency->code, diff --git a/app/Http/Controllers/ProfileController.php b/app/Http/Controllers/ProfileController.php index 32da7e878c..9c84f76e5a 100644 --- a/app/Http/Controllers/ProfileController.php +++ b/app/Http/Controllers/ProfileController.php @@ -555,7 +555,7 @@ class ProfileController extends Controller /** @var string $match */ $match = null; foreach ($set as $entry) { - $hashed = hash('sha256', $entry->data); + $hashed = hash('sha256', sprintf('%s%s', (string) config('app.key'), $entry->data)); if ($hashed === $hash) { $match = $entry->data; break; diff --git a/app/Repositories/ImportJob/ImportJobRepository.php b/app/Repositories/ImportJob/ImportJobRepository.php index b8ae64da08..58b7547589 100644 --- a/app/Repositories/ImportJob/ImportJobRepository.php +++ b/app/Repositories/ImportJob/ImportJobRepository.php @@ -384,7 +384,7 @@ class ImportJobRepository implements ImportJobRepositoryInterface $attachment = new Attachment; // create Attachment object. $attachment->user()->associate($job->user); $attachment->attachable()->associate($job); - $attachment->md5 = md5($content); + $attachment->md5 = substr(hash('sha256', $content), 0, 32); // limit due to DB. $attachment->filename = $name; $attachment->mime = 'plain/txt'; $attachment->size = strlen($content); diff --git a/app/Services/Password/PwndVerifierV2.php b/app/Services/Password/PwndVerifierV2.php index f1281b96ae..18c300e572 100644 --- a/app/Services/Password/PwndVerifierV2.php +++ b/app/Services/Password/PwndVerifierV2.php @@ -58,8 +58,11 @@ class PwndVerifierV2 implements Verifier $rest = substr($hash, 5); $uri = sprintf('https://api.pwnedpasswords.com/range/%s', $prefix); $opt = [ - 'headers' => ['User-Agent' => 'Firefly III v' . config('firefly.version')], - 'timeout' => 5]; + 'headers' => [ + 'User-Agent' => 'Firefly III v' . config('firefly.version'), + 'Add-Padding' => 'true', + ], + 'timeout' => 3.1415]; Log::debug(sprintf('hash prefix is %s', $prefix)); Log::debug(sprintf('rest is %s', $rest)); @@ -87,7 +90,7 @@ class PwndVerifierV2 implements Verifier return true; } - Log::debug(sprintf('Could not find %s, return FALSE.', $rest)); + Log::debug(sprintf('Found %s, return FALSE.', $rest)); return false; } diff --git a/app/Support/CacheProperties.php b/app/Support/CacheProperties.php index 4ae5edef1b..7472bc91c9 100644 --- a/app/Support/CacheProperties.php +++ b/app/Support/CacheProperties.php @@ -101,8 +101,8 @@ class CacheProperties { $content = ''; foreach ($this->properties as $property) { - $content .= json_encode($property); + $content .= json_encode($property, JSON_THROW_ON_ERROR, 512); } - $this->hash = substr(sha1($content), 0, 16); + $this->hash = substr(hash('sha256', $content), 0, 16); } } diff --git a/app/Support/Preferences.php b/app/Support/Preferences.php index f3d017b64d..42b231ba4b 100644 --- a/app/Support/Preferences.php +++ b/app/Support/Preferences.php @@ -26,7 +26,6 @@ use Cache; use Exception; use FireflyIII\Models\Preference; use FireflyIII\User; -use Illuminate\Contracts\Auth\Authenticatable; use Illuminate\Support\Collection; use Log; use Session; @@ -200,7 +199,7 @@ class Preferences $lastActivity = implode(',', $lastActivity); } - return md5($lastActivity); + return hash('sha256', $lastActivity); } /** diff --git a/database/migrations/2016_06_16_000002_create_main_tables.php b/database/migrations/2016_06_16_000002_create_main_tables.php index d4532a4871..0ca75aaf55 100644 --- a/database/migrations/2016_06_16_000002_create_main_tables.php +++ b/database/migrations/2016_06_16_000002_create_main_tables.php @@ -133,7 +133,7 @@ class CreateMainTables extends Migration $table->integer('user_id', false, true); $table->integer('attachable_id', false, true); $table->string('attachable_type', 255); - $table->string('md5', 32); + $table->string('md5', 128); $table->string('filename', 1024); $table->string('title', 1024)->nullable(); $table->text('description')->nullable(); From db5847b49b4e6f5378bce65d06273893e7ddc379 Mon Sep 17 00:00:00 2001 From: James Cole Date: Sat, 11 Apr 2020 06:42:40 +0200 Subject: [PATCH 2/9] Consistent in minimum password length --- app/Http/Requests/ProfileFormRequest.php | 2 +- app/Providers/FireflyServiceProvider.php | 4 +- app/Services/Password/PwndVerifierV3.php | 96 ------------------- .../Http/Controllers/RequestInformation.php | 2 +- .../views/v1/profile/change-password.twig | 2 +- 5 files changed, 5 insertions(+), 101 deletions(-) delete mode 100644 app/Services/Password/PwndVerifierV3.php diff --git a/app/Http/Requests/ProfileFormRequest.php b/app/Http/Requests/ProfileFormRequest.php index 72b7b00603..bc0ef0f7dc 100644 --- a/app/Http/Requests/ProfileFormRequest.php +++ b/app/Http/Requests/ProfileFormRequest.php @@ -50,7 +50,7 @@ class ProfileFormRequest extends Request // fixed return [ 'current_password' => 'required', - 'new_password' => 'required|confirmed|secure_password', + 'new_password' => 'required|confirmed|secure_password|min:16', 'new_password_confirmation' => 'required', ]; } diff --git a/app/Providers/FireflyServiceProvider.php b/app/Providers/FireflyServiceProvider.php index 67c82adc29..e489379798 100644 --- a/app/Providers/FireflyServiceProvider.php +++ b/app/Providers/FireflyServiceProvider.php @@ -48,7 +48,7 @@ use FireflyIII\Services\FireflyIIIOrg\Update\UpdateRequest; use FireflyIII\Services\FireflyIIIOrg\Update\UpdateRequestInterface; use FireflyIII\Services\IP\IpifyOrg; use FireflyIII\Services\IP\IPRetrievalInterface; -use FireflyIII\Services\Password\PwndVerifierV3; +use FireflyIII\Services\Password\PwndVerifierV2; use FireflyIII\Services\Password\Verifier; use FireflyIII\Support\Amount; use FireflyIII\Support\ExpandedForm; @@ -189,7 +189,7 @@ class FireflyServiceProvider extends ServiceProvider $this->app->bind(ExchangeRateInterface::class, $class); // password verifier thing - $this->app->bind(Verifier::class, PwndVerifierV3::class); + $this->app->bind(Verifier::class, PwndVerifierV2::class); // IP thing: $this->app->bind(IPRetrievalInterface::class, IpifyOrg::class); diff --git a/app/Services/Password/PwndVerifierV3.php b/app/Services/Password/PwndVerifierV3.php deleted file mode 100644 index 817d9ccf2c..0000000000 --- a/app/Services/Password/PwndVerifierV3.php +++ /dev/null @@ -1,96 +0,0 @@ -. - */ - -declare(strict_types=1); - -namespace FireflyIII\Services\Password; - - -use Exception; -use GuzzleHttp\Client; -use GuzzleHttp\Exception\GuzzleException; -use Log; -use RuntimeException; - -/** - * Class PwndVerifierV3 - * @codeCoverageIgnore - * @codeCoverageIgnore - * @deprecated - */ -class PwndVerifierV3 implements Verifier -{ - - /** - * Verify the given password against (some) service. - * - * @param string $password - * - * @return bool - */ - public function validPassword(string $password): bool - { - Log::debug('Now in API v3.'); - $hash = strtoupper(sha1($password)); - $prefix = substr($hash, 0, 5); - $rest = substr($hash, 5); - $uri = sprintf('https://api.pwnedpasswords.com/%s/%s', 'range', $prefix); - - Log::debug(sprintf('URI is %s', $uri)); - - $headers = [ - 'User-Agent' => sprintf('Firefly III v%s', config('firefly.version')), - ]; - Log::debug('Headers', $headers); - $opts = [ - 'headers' => $headers, - 'timeout' => 5, - ]; - - Log::debug(sprintf('hash prefix is %s', $prefix)); - Log::debug(sprintf('rest is %s', $rest)); - - try { - $client = new Client; - $res = $client->request('GET', $uri, $opts); - } catch (GuzzleException|Exception $e) { - Log::error(sprintf('Could not verify password security: %s', $e->getMessage())); - return true; - } - Log::debug(sprintf('Status code returned is %d', $res->getStatusCode())); - if (404 === $res->getStatusCode()) { - return true; - } - $body = $res->getBody()->getContents(); - try { - $strpos = stripos($body, $rest); - } catch (RuntimeException $e) { - Log::error(sprintf('Could not get body from Pwnd result: %s', $e->getMessage())); - $strpos = false; - } - if (false === $strpos) { - Log::debug(sprintf('%s was not found in result body. Return true.', $rest)); - return true; - } - Log::debug(sprintf('Found %s, so return FALSE.', $rest)); - return false; - } -} diff --git a/app/Support/Http/Controllers/RequestInformation.php b/app/Support/Http/Controllers/RequestInformation.php index a0110bd3b7..425c427f3c 100644 --- a/app/Support/Http/Controllers/RequestInformation.php +++ b/app/Support/Http/Controllers/RequestInformation.php @@ -300,7 +300,7 @@ trait RequestInformation $data, [ 'email' => 'required|string|email|max:255|unique:users', - 'password' => 'required|string|min:6|secure_password|confirmed', + 'password' => 'required|string|min:16|secure_password|confirmed', ] ); } diff --git a/resources/views/v1/profile/change-password.twig b/resources/views/v1/profile/change-password.twig index 2ea8d99b96..01832fc6a5 100644 --- a/resources/views/v1/profile/change-password.twig +++ b/resources/views/v1/profile/change-password.twig @@ -52,7 +52,7 @@ - {{ ExpandedForm.checkbox('verify_password','1', false) }} + {{ ExpandedForm.checkbox('verify_password','1', true) }}

{{ 'what_is_pw_security'|_ }}

From 4163efba55113ff4eef62c9494fa9bbf3ead3e92 Mon Sep 17 00:00:00 2001 From: James Cole Date: Sat, 11 Apr 2020 06:42:47 +0200 Subject: [PATCH 3/9] Clean up phpdoc --- app/Models/Budget.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/Models/Budget.php b/app/Models/Budget.php index 5a977ebdf2..94c118c490 100644 --- a/app/Models/Budget.php +++ b/app/Models/Budget.php @@ -77,6 +77,10 @@ use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; * @property-read int|null $budgetlimits_count * @property-read int|null $transaction_journals_count * @property-read int|null $transactions_count + * @property \Illuminate\Support\Carbon|null $created_at + * @property \Illuminate\Support\Carbon|null $updated_at + * @property bool $encrypted + * @property-read \Illuminate\Database\Eloquent\Collection|\FireflyIII\Models\BudgetLimit[] $budgetlimits */ class Budget extends Model { From 4a7d9b130adc23e139cbf3dd60eb08b1b6c4f16b Mon Sep 17 00:00:00 2001 From: James Cole Date: Sun, 12 Apr 2020 06:23:35 +0200 Subject: [PATCH 4/9] Fix issue with multi-currency in asset accounts. --- app/Http/Controllers/Chart/AccountController.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/Http/Controllers/Chart/AccountController.php b/app/Http/Controllers/Chart/AccountController.php index 487357342b..1e6b5dbb6e 100644 --- a/app/Http/Controllers/Chart/AccountController.php +++ b/app/Http/Controllers/Chart/AccountController.php @@ -432,9 +432,15 @@ class AccountController extends Controller $cache->addProperty($end); $cache->addProperty($account->id); if ($cache->has()) { - return response()->json($cache->get()); // @codeCoverageIgnore + return response()->json($cache->get()); // @codeCoverageIgnore } $currencies = $this->accountRepository->getUsedCurrencies($account); + + // if the account is not expense or revenue, just use the account's default currency. + if (!in_array($account->accountType->type, [AccountType::REVENUE, AccountType::EXPENSE], true)) { + $currencies= [$this->accountRepository->getAccountCurrency($account) ?? app('amount')->getDefaultCurrency()]; + } + /** @var TransactionCurrency $currency */ foreach ($currencies as $currency) { $chartData[] = $this->periodByCurrency($start, $end, $account, $currency); From 6daf083b3fa7356094357e50088c4082a37711cb Mon Sep 17 00:00:00 2001 From: James Cole Date: Sun, 12 Apr 2020 06:24:35 +0200 Subject: [PATCH 5/9] Clean up some code. --- .../Controllers/Chart/AccountController.php | 2 +- database/factories/ModelFactory.php | 2 +- .../assets/js/components/bills/Index.vue | 19 ++++++++++--------- .../components/passport/AuthorizedClients.vue | 7 ++++--- .../assets/js/components/passport/Clients.vue | 11 ++++++----- .../passport/PersonalAccessTokens.vue | 5 +++-- 6 files changed, 25 insertions(+), 21 deletions(-) diff --git a/app/Http/Controllers/Chart/AccountController.php b/app/Http/Controllers/Chart/AccountController.php index 1e6b5dbb6e..d6d1b4e786 100644 --- a/app/Http/Controllers/Chart/AccountController.php +++ b/app/Http/Controllers/Chart/AccountController.php @@ -438,7 +438,7 @@ class AccountController extends Controller // if the account is not expense or revenue, just use the account's default currency. if (!in_array($account->accountType->type, [AccountType::REVENUE, AccountType::EXPENSE], true)) { - $currencies= [$this->accountRepository->getAccountCurrency($account) ?? app('amount')->getDefaultCurrency()]; + $currencies = [$this->accountRepository->getAccountCurrency($account) ?? app('amount')->getDefaultCurrency()]; } /** @var TransactionCurrency $currency */ diff --git a/database/factories/ModelFactory.php b/database/factories/ModelFactory.php index aea14f83cc..d52f908a3c 100644 --- a/database/factories/ModelFactory.php +++ b/database/factories/ModelFactory.php @@ -30,7 +30,7 @@ $factory->define( 'user_id' => 1, 'attachable_id' => 1, 'attachable_type' => TransactionJournal::class, - 'md5' => md5($faker->words(6, true)), + 'md5' => substr(hash('sha256', $faker->words(6, true)), 0, 32), 'mime' => 'text/plain', 'size' => 1, 'filename' => 'ok', diff --git a/resources/assets/js/components/bills/Index.vue b/resources/assets/js/components/bills/Index.vue index 3c702e6183..1ff1c90048 100644 --- a/resources/assets/js/components/bills/Index.vue +++ b/resources/assets/js/components/bills/Index.vue @@ -20,17 +20,18 @@