Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions app/Http/Controllers/GroupController.php
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ public function delete($id): RedirectResponse
$group->delete();

return redirect('/group')->with('success', __('groups.delete_succeeded', [
'name' => $name,
'name' => e($name),
]));
} else {
return redirect('/user/forbidden');
Expand Down Expand Up @@ -576,7 +576,7 @@ public function getJoinGroup($group_id): RedirectResponse
return redirect()
->back()
->with('success', __('groups.now_following', [
'name' => $group->name,
'name' => e($group->name),
'link' => url('/group/view/'.$group->idgroups),
]));
} catch (\Exception $e) {
Expand Down Expand Up @@ -661,7 +661,7 @@ public function inviteNearbyRestarter($groupId, $userId): RedirectResponse
}
}

return redirect('/group/nearby/'.intval($groupId))->with('success', $user->name.' has been invited');
return redirect('/group/nearby/'.intval($groupId))->with('success', e($user->name).' has been invited');
}

/**
Expand Down
2 changes: 1 addition & 1 deletion app/Http/Controllers/UserController.php
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ public function postSoftDeleteUser(Request $request): RedirectResponse

if (Auth::id() !== $user_id) {
return redirect('user/all')->with('danger', __('profile.soft_deleted', [
'name' => $old_user_name
'name' => e($old_user_name)
]));
} else {
return redirect('login');
Expand Down
8 changes: 4 additions & 4 deletions app/Http/Middleware/AcceptUserInvites.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,13 @@ public function handle(Request $request, Closure $next): Response
$acceptance->delete();
$request->session()->push('invites-feedback', __('groups.you_have_joined', [
'url' => url("/group/view/{$group->idgroups}"),
'name' => $group->name
'name' => e($group->name)
]));

// Else that must mean the User is already part of the Group.
// We can then delete the Invite and create a new session
} else {
$request->session()->push('invites-feedback', 'You are already a member of <a class="plain-link" href='.url("/group/view/{$group->idgroups}").">{$group->name}</a>");
$request->session()->push('invites-feedback', 'You are already a member of <a class="plain-link" href="'.url("/group/view/{$group->idgroups}").'">'.e($group->name).'</a>');
}
}
$request->session()->forget('groups');
Expand All @@ -78,13 +78,13 @@ public function handle(Request $request, Closure $next): Response
$acceptance->delete();
$request->session()->push('invites-feedback', __('events.you_have_joined', [
'url' => url("/party/view/{$event->idevents}"),
'name' => $event->venue
'name' => e($event->venue)
]));

// Else that must mean the User is already part of the Event.
// We can then delete the Invite and create a new session
} else {
$request->session()->push('invites-feedback', 'You are already a member of <a class="plain-link" href='.url("/party/view/{$event->idevents}").">{$event->venue}</a>");
$request->session()->push('invites-feedback', 'You are already a member of <a class="plain-link" href="'.url("/party/view/{$event->idevents}").'">'.e($event->venue).'</a>');
}
}
$request->session()->forget('events');
Expand Down
5 changes: 5 additions & 0 deletions app/Models/Brands.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,9 @@ class Brands extends Model
* @var array
*/
protected $hidden = [];

public function setBrandNameAttribute($value)
{
$this->attributes['brand_name'] = $value === null ? null : strip_tags((string) $value);
}
}
5 changes: 5 additions & 0 deletions app/Models/Category.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ class Category extends Model

// Setters

public function setNameAttribute($value)
{
$this->attributes['name'] = $value === null ? null : strip_tags((string) $value);
}

//Getters
public function findAll()
{
Expand Down
11 changes: 11 additions & 0 deletions app/Models/Group.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use Illuminate\Database\Eloquent\Model;
use Illuminate\Support\Facades\Lang;
use Illuminate\Support\Facades\Log;
use Stevebauman\Purify\Facades\Purify;
use OwenIt\Auditing\Contracts\Auditable;
use Illuminate\Database\Eloquent\SoftDeletes;

Expand Down Expand Up @@ -446,6 +447,16 @@ public function setDistanceAttribute($val)
$this->distance = $val;
}

public function setNameAttribute($value)
{
$this->attributes['name'] = $value === null ? null : strip_tags((string) $value);
}

public function setFreeTextAttribute($value)
{
$this->attributes['free_text'] = $value === null ? null : Purify::clean((string) $value);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to sanitize input text is a mistake I think. This concept has been around for decades, and the solution is almost always: Treat all input as dangerous, and thus escape appropriately it when outputting.

Why are we trying to clean the human input here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a safety measure, we've audited the code and until we address every location, we are taking the extra step to sanitize the input as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the main reason to clean human input going forward (where we want to end up) would be if it's allowed to contain HTML. I think that may be applicable here (edited via rich text editor).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the main reason to clean human input going forward (where we want to end up) would be if it's allowed to contain HTML

That evidence sounds like an argument for not trying to sanitize inputs.

Even if that's the case (we accept html-containing input), I still think it's better to keep the original value and strip / sanitize on output.

Rules about what to strip / sanitize change, which makes it much harder to retroactively apply those changes if we are depending on DB content already being safe.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've convinced me it's better to sanitize on output.

}

public function createDiscourseGroup() {
// Get the host who created the group.
$success = false;
Expand Down
5 changes: 5 additions & 0 deletions app/Models/GroupTags.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,10 @@ public function groupTagGroups(): HasMany

// Setters

public function setTagNameAttribute($value)
{
$this->attributes['tag_name'] = $value === null ? null : strip_tags((string) $value);
}

//Getters
}
11 changes: 11 additions & 0 deletions app/Models/Network.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,22 @@
use Illuminate\Database\Eloquent\Factories\HasFactory;
use App\Models\Group;
use Illuminate\Database\Eloquent\Model;
use Stevebauman\Purify\Facades\Purify;

class Network extends Model
{
use HasFactory;

public function setNameAttribute($value)
{
$this->attributes['name'] = $value === null ? null : strip_tags((string) $value);
}

public function setDescriptionAttribute($value)
{
$this->attributes['description'] = $value === null ? null : Purify::clean((string) $value);
}

public function groups(): BelongsToMany
{
return $this->belongsToMany(Group::class, 'group_network', 'network_id', 'group_id');
Expand Down
11 changes: 11 additions & 0 deletions app/Models/Party.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use DB;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\SoftDeletes;
use Stevebauman\Purify\Facades\Purify;
use Illuminate\Support\Str;
use Notification;
use OwenIt\Auditing\Contracts\Auditable;
Expand Down Expand Up @@ -767,6 +768,16 @@ public function getEventEndUtcAttribute() {
return array_key_exists('event_end_utc', $this->attributes) ? Carbon::parse($this->attributes['event_end_utc'], 'UTC')->toIso8601String() : null;
}

public function setVenueAttribute($value)
{
$this->attributes['venue'] = $value === null ? null : strip_tags((string) $value);
}

public function setFreeTextAttribute($value)
{
$this->attributes['free_text'] = $value === null ? null : Purify::clean((string) $value);
}

// Mutators for previous event_date/start/end fields. These are now superceded by the UTC fields and therefore
// should never be set directly. Throw exceptions to ensure that they are not.
public function setEventDateAttribute($val) {
Expand Down
5 changes: 5 additions & 0 deletions app/Models/Skills.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,10 @@ class Skills extends Model

// Setters

public function setSkillNameAttribute($value)
{
$this->attributes['skill_name'] = $value === null ? null : strip_tags((string) $value);
}

//Getters
}
16 changes: 16 additions & 0 deletions app/Models/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Illuminate\Database\Eloquent\Factories\HasFactory;
use App\Events\UserDeleted;
use App\Events\UserUpdated;
use Stevebauman\Purify\Facades\Purify;
use App\Helpers\Fixometer;
use App\Models\Network;
use App\Models\UserGroups;
Expand Down Expand Up @@ -64,6 +65,21 @@ class User extends Authenticatable implements Auditable, HasLocalePreference
'remember_token',
];

public function setNameAttribute($value)
{
$this->attributes['name'] = $value === null ? null : strip_tags((string) $value);
}

public function setLocationAttribute($value)
{
$this->attributes['location'] = $value === null ? null : strip_tags((string) $value);
}

public function setBiographyAttribute($value)
{
$this->attributes['biography'] = $value === null ? null : Purify::clean((string) $value);
}

/**
* The event map for the model.
*
Expand Down
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,10 @@
"sentry/sentry-laravel": "^4.13",
"soundasleep/html2text": "^1.1",
"spatie/calendar-links": "^1.6",
"spatie/laravel-validation-rules": "^3.4",
"spatie/laravel-html": "^3.12.0",
"spatie/laravel-validation-rules": "^3.4",
"spinen/laravel-discourse-sso": "dev-l12-compatibility",
"stevebauman/purify": "^6.3",
"symfony/http-client": "^7.2",
"symfony/http-foundation": "^7.2",
"symfony/mailgun-mailer": "^7.2",
Expand Down
131 changes: 129 additions & 2 deletions composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading