fix: Security hardening for auth and token flows

app/Http/Controllers/Discord/Registration.php

@@ -29,13 +29,19 @@ public function create(Request $request)
         $authUrl = $this->provider->getAuthorizationUrl([
             'scope' => ['identify', 'guilds.join'],
         ]);
+        $request->session()->put('discordauthstate', $this->provider->getState());
 
         return redirect()->away($authUrl);
     }
 
     public function store(DiscordRegistration $request)
     {
         $inputs = $request->validated();
+        $storedState = session()->pull('discordauthstate', '');
+
+        if (! is_string($storedState) || $storedState === '' || ! hash_equals($storedState, $inputs['state'])) {
+            return $this->error('Something went wrong. Please try again.');
+        }
 
         try {
             $token = $this->provider->getAccessToken('authorization_code', ['code' => $inputs['code']]);

app/Http/Requests/Mship/Account/DiscordRegistration.php

@@ -25,6 +25,7 @@ public function rules()
     {
         return [
             'code' => 'required|string',
+            'state' => 'required|string',
         ];
     }
 }

app/Libraries/UKCP.php

@@ -151,10 +151,9 @@ public function markNotificationReadForUser(Account $account, int $notificationI
 
             return true;
         } catch (ClientException $e) {
-            dd($e);
             Log::info("UKCP Client Exception {$e->getMessage()} when marking notification read");
 
-            return [];
+            return false;
         }
     }
 

app/Models/Sys/Token.php

@@ -123,7 +123,7 @@ public static function generate($type, $allowDuplicates = false, $relation = nul
         $token = new self;
         $token->type = $type;
         $token->expires_at = \Carbon\Carbon::now()->addMinutes($expireMinutes)->toDateTimeString();
-        $token->code = uniqid(uniqid());
+        $token->code = self::generateSecureCode();
 
         if ($relation != null) {
             $relation->tokens()->save($token);
@@ -134,6 +134,16 @@ public static function generate($type, $allowDuplicates = false, $relation = nul
         return $token;
     }
 
+    private static function generateSecureCode(): string
+    {
+        do {
+            // `sys_token.code` is VARCHAR(31), so keep this at 30 chars.
+            $code = bin2hex(random_bytes(15));
+        } while (self::where('code', '=', $code)->exists());
+
+        return $code;
+    }
+
     public function consume()
     {
         if (! $this || $this->is_used || $this->is_expired) {
@@ -146,7 +156,7 @@ public function consume()
 
     public function getIsUsedAttribute()
     {
-        return $this->used_at != null && $this->used_at->isPast();
+        return $this->used_at != null;
     }
 
     public function getIsExpiredAttribute()

app/Providers/AppServiceProvider.php

@@ -134,9 +134,9 @@ public function registerValidatorExtensions()
 
         Validator::extend('numbers', function ($attribute, $value, $parameters, $validator) {
             if (isset($parameters[0])) {
-                return str_has_lower($value, $parameters[0]);
+                return str_has_numeric($value, $parameters[0]);
             } else {
-                return str_has_lower($value);
+                return str_has_numeric($value);
             }
         });
 

resources/views/mship/management/dashboard.blade.php

@@ -40,14 +40,18 @@
                 <div class="panel-heading"><i class="fa fa-male"></i> &thinsp; Personal Details
 
                     <div class="pull-right">
-                        <a
-                        class="tooltip_displays"
-                        href="{{ route('mship.manage.cert.update') }}"
-                        data-toggle="tooltip"
-                        title="{{ !is_null($_account->cert_checked_at) ? 'Last updated with VATSIM.net ' . $_account->cert_checked_at->diffForHumans() : 'Not yet updated with VATSIM.net.' }} "
-                        >
-                            <i class="fa fa-sync"></i>
-                        </a>
+                        <form method="POST" action="{{ route('mship.manage.cert.update') }}" style="display: inline;">
+                            @csrf
+                            <button
+                                type="submit"
+                                class="tooltip_displays"
+                                style="border: none; background: transparent; padding: 0;"
+                                data-toggle="tooltip"
+                                title="{{ !is_null($_account->cert_checked_at) ? 'Last updated with VATSIM.net ' . $_account->cert_checked_at->diffForHumans() : 'Not yet updated with VATSIM.net.' }} "
+                            >
+                                <i class="fa fa-sync"></i>
+                            </button>
+                        </form>
                     </div>
                 </div>
                 <div class="panel-body">
@@ -105,9 +109,12 @@ class="tooltip_displays"
                 <div class="panel-footer panel-footer-primary">
                     <div class="row">
                         <div class="col-xs-12">
-                            <a href="{{ route('mship.manage.cert.update') }}">
-                                <span class='fa fa-sync'></span> Details look incorrect? Click here to request an update from VATSIM.net.
-                            </a>
+                            <form method="POST" action="{{ route('mship.manage.cert.update') }}">
+                                @csrf
+                                <button type="submit" class="btn btn-link" style="padding: 0;">
+                                    <span class='fa fa-sync'></span> Details look incorrect? Click here to request an update from VATSIM.net.
+                                </button>
+                            </form>
                         </div>
                     </div>
                 </div>
@@ -245,14 +252,18 @@ class="btn btn-xs btn-danger">
                 <div class="panel-heading"><i class="fa fa-graduation-cap"></i> &thinsp; ATC & Pilot Qualifications
 
                     <div class="pull-right">
-                        <a
-                        class="tooltip_displays"
-                        href="{{ route('mship.manage.cert.update') }}"
-                        data-toggle="tooltip"
-                        title="{{ !is_null($_account->cert_checked_at) ? 'Last updated with VATSIM.net ' . $_account->cert_checked_at->diffForHumans() : 'Not yet updated with VATSIM.net.' }} "
-                        >
-                            <i class="fa fa-sync"></i>
-                        </a>
+                        <form method="POST" action="{{ route('mship.manage.cert.update') }}" style="display: inline;">
+                            @csrf
+                            <button
+                                type="submit"
+                                class="tooltip_displays"
+                                style="border: none; background: transparent; padding: 0;"
+                                data-toggle="tooltip"
+                                title="{{ !is_null($_account->cert_checked_at) ? 'Last updated with VATSIM.net ' . $_account->cert_checked_at->diffForHumans() : 'Not yet updated with VATSIM.net.' }} "
+                            >
+                                <i class="fa fa-sync"></i>
+                            </button>
+                        </form>
                     </div>
                 </div>
                 <div class="panel-body">
@@ -334,9 +345,12 @@ class="tooltip_displays"
                 <div class="panel-footer panel-footer-primary">
                     <div class="row">
                         <div class="col-xs-12">
-                            <a href="{{ route('mship.manage.cert.update') }}">
-                                <span class='fa fa-sync'></span> Details look incorrect? Click here to request an update from VATSIM.net.
-                            </a>
+                            <form method="POST" action="{{ route('mship.manage.cert.update') }}">
+                                @csrf
+                                <button type="submit" class="btn btn-link" style="padding: 0;">
+                                    <span class='fa fa-sync'></span> Details look incorrect? Click here to request an update from VATSIM.net.
+                                </button>
+                            </form>
                         </div>
                     </div>
                 </div>
@@ -381,8 +395,12 @@ class="tooltip_displays"
                                                 <strong>LAST LOGIN</strong>: {{ $tsreg->last_login }}<br/>
                                                 <strong>OPERATING SYSTEM</strong>: {{ $tsreg->last_os }}<br/>
                                             @endif
-                                            [ <a href="{{ route('teamspeak.delete', [$tsreg->id]) }}">Remove Registration</a>
-                                            ]<br/>&nbsp;
+                                            [ <form method="POST" action="{{ route('teamspeak.delete', [$tsreg->id]) }}" style="display: inline;">
+                                                @csrf
+                                                <button type="submit" class="btn btn-link" style="padding: 0; vertical-align: baseline;">
+                                                    Remove Registration
+                                                </button>
+                                            </form> ]<br/>&nbsp;
                                         </div>
                                     @endforeach
                                 </div>
@@ -433,7 +451,14 @@ class="tooltip_displays"
                                         Currently registered with Discord account {{ $_account->discord_user ? '@' . $_account->discord_user['username'] : $_account->discord_id }}.
                                     </button>
                                 </a>
-                                <p class="text-center"><a href="{{ route('discord.destroy') }}">Unlink Account</a></p>
+                                <div class="text-center">
+                                    <form method="POST" action="{{ route('discord.destroy') }}">
+                                        @csrf
+                                        <button type="submit" class="btn btn-link" style="padding: 0;">
+                                            Unlink Account
+                                        </button>
+                                    </form>
+                                </div>
                             @endif
                         </div>
                     </div>
@@ -450,9 +475,12 @@ class="tooltip_displays"
                             <b>UK CONTROLLER<br/>PLUGIN KEYS</b>
                             @if(count($pluginKeys))
                                 <div class="text-center pt-4">
-                                    <a class="btn btn-warning btn-sm" href="{{ route('ukcp.token.invalidate') }}">
-                                        Invalidate Token(s)
-                                    </a>
+                                    <form method="POST" action="{{ route('ukcp.token.invalidate') }}">
+                                        @csrf
+                                        <button type="submit" class="btn btn-warning btn-sm">
+                                            Invalidate Token(s)
+                                        </button>
+                                    </form>
                                     </br>
                                     <small>
                                         Note: If you are currently online, some operations, such as squawk assignments, will fail.

resources/views/teamspeak/new.blade.php

@@ -5,7 +5,15 @@
 <div class="row">
     @if (array_get($_SERVER, 'REMOTE_ADDR') != $registration->registration_ip)
         <div class="alert alert-danger" role="alert"><strong>Warning!</strong> Your current IP address ({{ array_get($_SERVER, 'REMOTE_ADDR') }}) is different to the IP address you used to create this registration ({{ $registration->registration_ip }}).<br>
-            To successfully register, your current IP address must be identical to the one you used to create this registration. <strong><a href="{{ route('teamspeak.delete', [$registration->id]) }}" class="alert-link">Click here</a></strong> to start a new registration.</div>
+            To successfully register, your current IP address must be identical to the one you used to create this registration.
+            <form method="POST" action="{{ route('teamspeak.delete', [$registration->id]) }}" style="display: inline;">
+                @csrf
+                <button type="submit" class="alert-link" style="border: none; background: transparent; padding: 0;">
+                    <strong>Click here</strong>
+                </button>
+            </form>
+            to start a new registration.
+        </div>
     @endif
 
     <div class="alert alert-danger" role="alert" id="helpmessage" style="display:none">

routes/web-main.php

@@ -56,7 +56,7 @@
         'prefix' => 'manage',
     ], function () {
         Route::get('dashboard')->uses('Management@getDashboard')->name('dashboard');
-        Route::get('cert/update')->uses('Management@requestCertCheck')->name('cert.update');
+        Route::post('cert/update')->uses('Management@requestCertCheck')->name('cert.update');
         Route::get('email/verify/{code}')->uses('Management@getVerifyEmail')->name('email.verify');
         Route::get('email/add')->uses('Management@getEmailAdd')->name('email.add');
         Route::post('email/add')->uses('Management@postEmailAdd')->name('email.add.post');
@@ -125,7 +125,7 @@
     Route::model('tsreg', App\Models\TeamSpeak\Registration::class);
     Route::get('new', ['as' => 'teamspeak.new', 'uses' => 'Registration@getNew']);
     Route::get('success', ['as' => 'teamspeak.success', 'uses' => 'Registration@getConfirmed']);
-    Route::get('{mshipRegistration}/delete', ['as' => 'teamspeak.delete', 'uses' => 'Registration@getDelete']);
+    Route::post('{mshipRegistration}/delete', ['as' => 'teamspeak.delete', 'uses' => 'Registration@getDelete']);
     Route::post('{mshipRegistration}/status', ['as' => 'teamspeak.status', 'uses' => 'Registration@postStatus']);
 });
 
@@ -139,7 +139,7 @@
     Route::get('/')->uses('Registration@show')->name('show');
     Route::get('/create')->uses('Registration@create')->name('create');
     Route::get('/store')->uses('Registration@store')->name('store');
-    Route::get('/destroy')->uses('Registration@destroy')->name('destroy');
+    Route::post('/destroy')->uses('Registration@destroy')->name('destroy');
 });
 
 // UKCP
@@ -150,7 +150,7 @@
     'middleware' => 'auth_full_group',
 ], function () {
     Route::get('/')->uses('Token@show')->name('guide');
-    Route::get('/token/invalidate')->uses('Token@invalidate')->name('token.invalidate');
+    Route::post('/token/invalidate')->uses('Token@invalidate')->name('token.invalidate');
 });
 
 // Controllers

tests/Feature/Account/ResetPasswordTest.php

@@ -55,4 +55,23 @@ public function test_password_reset_updates_correctly()
         $this->assertEquals($now, $this->user->fresh()->password_set_at);
         $this->assertEquals($now->addDays($this->user->roles()->first()->password_lifetime), $this->user->fresh()->password_expires_at);
     }
+
+    #[Test]
+    public function test_password_reset_rejects_password_without_numeric_character()
+    {
+        $this->user->setPassword('Testing123');
+        $token = Password::broker()->createToken($this->user);
+
+        $this->actingAs($this->user, 'vatsim-sso')
+            ->from(route('password.reset', $token))
+            ->post(route('password.request'), [
+                'token' => $token,
+                'new_password' => 'PasswordNoDigits',
+                'new_password_confirmation' => 'PasswordNoDigits',
+            ])
+            ->assertRedirect(route('password.reset', $token))
+            ->assertSessionHasErrors('new_password');
+
+        $this->assertTrue(Hash::check('Testing123', $this->user->fresh()->password));
+    }
 }

tests/Feature/Account/TeamspeakManagementTest.php

@@ -17,15 +17,15 @@ class TeamspeakManagementTest extends TestCase
     public function test_user_can_delete_own_registration()
     {
         $this->from(route('mship.manage.dashboard'))->followingRedirects()->actingAs($this->registration->account)
-            ->get(route('teamspeak.delete', ['mshipRegistration' => $this->registration->id]))
+            ->post(route('teamspeak.delete', ['mshipRegistration' => $this->registration->id]))
             ->assertSuccessful();
     }
 
     #[Test]
     public function test_user_cant_delete_others_registration()
     {
         $this->followingRedirects()->actingAs($this->user)
-            ->get(route('teamspeak.delete', $this->registration))
+            ->post(route('teamspeak.delete', $this->registration))
             ->assertNotFound();
     }
 
@@ -49,6 +49,14 @@ protected function setUp(): void
     {
         parent::setUp();
 
+        config([
+            'services.teamspeak.host' => null,
+            'services.teamspeak.username' => null,
+            'services.teamspeak.password' => null,
+            'services.teamspeak.port' => null,
+            'services.teamspeak.query_port' => null,
+        ]);
+
         $this->registration = factory(Registration::class)->create();
     }
 }

tests/Feature/Services/DiscordTest.php

@@ -91,6 +91,31 @@ public function test_it_redirects_when_code_missing()
         $nullCode->assertRedirect(route('mship.manage.dashboard'))->assertSessionHasErrors('code');
     }
 
+    #[Test]
+    public function test_it_redirects_when_state_missing()
+    {
+        $this->actingAs($this->user)
+            ->withSession(['discordauthstate' => 'expected-state'])
+            ->from(route('mship.manage.dashboard'))
+            ->get(route('discord.store', ['code' => '123456789']))
+            ->assertRedirect(route('mship.manage.dashboard'))
+            ->assertSessionHasErrors('state');
+    }
+
+    #[Test]
+    public function test_it_rejects_request_when_state_is_invalid()
+    {
+        $this->actingAs($this->user)
+            ->withSession(['discordauthstate' => 'expected-state'])
+            ->from(route('mship.manage.dashboard'))
+            ->get(route('discord.store', [
+                'code' => '123456789',
+                'state' => 'unexpected-state',
+            ]))
+            ->assertRedirect(route('mship.manage.dashboard'))
+            ->assertSessionHas('error', 'Something went wrong. Please try again.');
+    }
+
     #[Test]
     public function test_it_reports_when_user_in_too_many_servers()
     {
@@ -106,8 +131,10 @@ public function test_it_reports_when_user_in_too_many_servers()
         ]);
 
         $this->actingAs($this->user)
+            ->withSession(['discordauthstate' => 'valid-state'])
             ->get(route('discord.store', [
                 'code' => '123456789',
+                'state' => 'valid-state',
             ]))
             ->assertRedirect(route('mship.manage.dashboard'))
             ->assertSessionHas('error', 'You have reached your Discord server limit! You must leave a server before you can join another one');