Skip to content

Conversation

@sebastienfontaine
Copy link
Contributor

No description provided.

'driver' => 'custom',
'path' => DBHandler::class,
'via' => DBLogger::class,
'path' => \FuryBee\LoggerUi\DBHandler::class,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

'path' => \FuryBee\LoggerUi\DBHandler::class,
'via' => \FuryBee\LoggerUi\DBLogger::class,

L'utilisation du FQCN (Fully Qualified Class Name) améliore la clarté, mais assurez-vous que ces classes existent bien dans le namespace indiqué et qu'il n'y a pas de conflit avec d'autres classes portant le même nom. Sinon, cela pourrait causer des erreurs d'autoload ou des bugs d'exécution.

v-html="value"
class="whitespace-normal"
v-if="key !== 'exception'"
v-html="value"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

v-html="value"

L'utilisation de v-html sans échapper ni filtrer les données peut exposer à des failles XSS si value contient du contenu utilisateur non contrôlé. Vérifiez que value est toujours sûr ou appliquez un filtrage/sanitation côté serveur ou client.

class="whitespace-normal"
v-html="subValue"
class="whitespace-normal"
v-html="subValue"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

v-html="subValue"

L'utilisation de v-html sans échapper ni filtrer les données peut exposer à des failles XSS si subValue contient du contenu utilisateur non contrôlé. Vérifiez que subValue est toujours sûr ou appliquez un filtrage/sanitation côté serveur ou client.

},
methods: {
toggleFilterBar() {
let filterBar = document.getElementById('filterBar');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

let filterBar = document.getElementById('filterBar');
filterBar.classList.toggle('hidden');

Aucune vérification que filterBar n'est pas null avant d'appeler classList.toggle. Cela peut causer une exception si l'élément n'existe pas dans le DOM. Ajoutez une vérification de nullité pour éviter une erreur d'exécution.


$table->string('app_name')->index();
$table->string('environnement')->index();
$table->string('environment')->index();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$table->string('environment')->index();

Le renommage de 'environnement' en 'environment' peut casser la rétrocompatibilité avec les données existantes ou le code qui s'appuie sur l'ancien nom de colonne. Vérifier que toutes les références à cette colonne sont bien mises à jour dans l'application et que la migration de données est gérée.

'context' => $record['context'],
'extra' => json_encode($record['extra']),
'user_id' => auth()->id(),
'user_id' => optional(auth())->id(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

'user_id'       => optional(auth())->id(),

L'utilisation de optional(auth())->id() évite une exception si l'utilisateur n'est pas authentifié, mais cela peut entraîner l'enregistrement de logs sans utilisateur associé (user_id null). Selon la sensibilité des logs, il peut être nécessaire de vérifier explicitement la présence d'un utilisateur ou de gérer ce cas pour éviter une perte de traçabilité ou une faille d'audit.

$builder->where(function (Builder $builder) use ($query) {
$builder->where('message', 'LIKE', "%{$query}%");
$builder->orWhere('context', 'LIKE', "%{$query}%");
$builder->orWhere('extra', 'LIKE', "%{$query}%");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$builder->orWhere('extra', 'LIKE', "%{$query}%");

Interpolation directe de la variable $query dans une clause SQL peut exposer à des injections SQL. Utilisez les bindings Laravel :

$builder->orWhere('extra', 'LIKE', "%" . $query . "%");

Mais idéalement :

$builder->orWhere('extra', 'LIKE', DB::raw('?', ["%{$query}%"]));

Assurez-vous que $query est correctement échappé ou passé en paramètre lié.

->getBuilder()
->selectRaw('DISTINCT(environnement) as environnements')
->pluck('environnements')
->selectRaw('DISTINCT(environment) as environments')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

->selectRaw('DISTINCT(environment) as environments')

Le renommage de 'environnement' en 'environment' et 'environnements' en 'environments' doit être cohérent avec le schéma de la base de données. Vérifier que le champ 'environment' existe bien, sinon cela provoquera une erreur d'exécution.

->selectRaw('DISTINCT(environnement) as environnements')
->pluck('environnements')
->selectRaw('DISTINCT(environment) as environments')
->pluck('environments')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

->pluck('environments')

Assurez-vous que l'alias 'environments' correspond bien au nom utilisé dans selectRaw, sinon pluck retournera une collection vide.

* @throws Throwable
*/
private function ensureEnvExists()
private function ensureEnvExists(): bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

private function ensureEnvExists(): bool

L'ajout du type de retour bool améliore la clarté et la sécurité du typage de la méthode.

{
if ($this->app->runningInConsole()) {
return;
return false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

return false;

Le retour explicite de false est préférable à un simple return; pour une méthode typée bool.

return isset($dbConfig)
&& isset($dbConfig['connection'])
&& isset($dbConfig['table']);
return isset($dbConfig) === true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

return isset($dbConfig) === true
    && isset($dbConfig['connection']) === true
    && isset($dbConfig['table']) === true;

L'ajout de === true est redondant car isset() retourne déjà un booléen. Cela nuit à la lisibilité sans apporter de bénéfice fonctionnel.

@sebastienfontaine
Copy link
Contributor Author

Voici la consolidation des remarques importantes issues de l'analyse de la Merge Request :

  1. Sécurité et robustesse
  • Utilisation de v-html dans Vue.js : Plusieurs occurrences de v-html sont utilisées sans filtrage ni échappement des données (ex : value, subValue). Cela expose à des risques XSS si les données proviennent de l'utilisateur. Il est impératif de s'assurer que ces valeurs sont sûres ou de mettre en place un filtrage/sanitation côté serveur ou client.
  • Interpolation directe dans les requêtes SQL : L'injection directe de variables dans les clauses SQL (ex : $builder->orWhere('extra', 'LIKE', "%{$query}%");) expose à des risques d'injection SQL. Il est recommandé d'utiliser les bindings de paramètres de Laravel ou de s'assurer que la variable est correctement échappée.
  • Suppression de la levée d'exception en cas d'environnement manquant : La suppression de ce bloc retire une protection importante contre une mauvaise configuration, ce qui peut entraîner des erreurs silencieuses ou des comportements inattendus. Il est conseillé de conserver cette vérification pour garantir la robustesse du service.
  • Vérification de nullité dans le DOM : L'accès à un élément DOM sans vérifier sa présence (ex : filterBar.classList.toggle('hidden');) peut provoquer des exceptions si l'élément n'existe pas. Ajoutez une vérification de nullité avant manipulation.
  1. Cohérence et rétrocompatibilité
  • Renommage de colonnes (environnement → environment) : Ce changement peut casser la rétrocompatibilité avec les données existantes ou le code qui s'appuie sur l'ancien nom. Il faut s'assurer que toutes les références sont bien mises à jour et que la migration de données est correctement gérée.
  • Cohérence des alias dans les requêtes : Lors de l'utilisation de selectRaw et pluck, vérifiez que les alias utilisés correspondent bien et que les champs existent dans le schéma de la base de données pour éviter des erreurs d'exécution ou des résultats inattendus.
  1. Bonnes pratiques de code
  • Utilisation du FQCN : L'utilisation du Fully Qualified Class Name améliore la clarté, mais il faut s'assurer que les classes existent bien dans le namespace indiqué et qu'il n'y a pas de conflit de nommage.
  • Utilisation de optional(auth())->id() : Cette pratique évite une exception si l'utilisateur n'est pas authentifié, mais peut entraîner des logs sans utilisateur associé (user_id null). Selon la sensibilité des logs, il peut être nécessaire de gérer explicitement ce cas pour éviter une perte de traçabilité.
  • Typage et retour explicite : L'ajout du type de retour bool sur une méthode améliore la clarté, mais l'ajout de === true après isset() est redondant et nuit à la lisibilité.

Résumé des problèmes communs :

  • Risques de sécurité (XSS, injection SQL, perte de traçabilité)
  • Problèmes de robustesse (suppression de protections, absence de vérification de nullité)
  • Risques de rupture de compatibilité lors de renommages
  • Manque de cohérence dans l'utilisation des alias et des noms de colonnes
  • Quelques points de style et de lisibilité du code

Merci de prendre en compte ces points pour améliorer la qualité et la sécurité de la Merge Request.

@sebastienfontaine
Copy link
Contributor Author

Remarques globales non positionnables :

  • src/LoggerUiServiceProvider.php, ligne 21 : La suppression du bloc qui lançait une exception en cas d'environnement manquant supprime la protection contre une mauvaise configuration. Cela peut entraîner des erreurs silencieuses ou des comportements inattendus si l'environnement n'est pas correctement défini. Il est recommandé de conserver la levée d'exception pour garantir la sécurité et la robustesse du service.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR standardizes the use of the environment field across the database, code, and UI, refactors the ensureEnvExists() method to return a boolean, and introduces UI enhancements for full-height layout and mobile filter toggling.

  • Renamed all occurrences of environnement to environment in SQL, migrations, repository, controllers, and views.
  • Updated ensureEnvExists() signature and removed its exception logic.
  • Enhanced the frontend: added full-height classes, a mobile filter toggle button, extra-field search, and an element-based scroll-to in logs.

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/LoggerUiServiceProvider.php Changed ensureEnvExists() to return bool and removed exception
src/Http/Repositories/LogRepository.php Renamed query field to environment
src/Http/Controllers/LogController.php Updated filters to use environment and added extra to search
src/DBHandler.php Renamed payload key to environment and used optional(auth())
src/Console/sql/2021_11_11_000001_create_logger_ui_table_singlestore.sql Renamed column to environment
src/Console/MigrateCommand.php Renamed migration column to environment
resources/views/layout.blade.php Added full-height (h-full) classes
resources/js/pages/home/partials/filter-bar.vue Wrapped template in a container, added toggle button and classes
resources/js/pages/home/index.vue Changed layout height, environment filter, and scroll implementation
public/mix-manifest.json Updated /app.js cache-busting hash
composer.json Added Laravel ^9.0 support and ext-json requirement
README.md Clarified SingleStore and queue instructions
.gitignore copy Deleted redundant copy
Comments suppressed due to low confidence (3)

src/Http/Controllers/LogController.php:51

  • The docblock specifies @return array but the method returns a JsonResponse. Please update the docblock to reflect the actual return type.
*     @return array

README.md:22

  • [nitpick] The instruction to "modify --singlestore option" is ambiguous. It would help to specify the expected flag value (e.g., --singlestore=on|off).
Note : if you are using SingleStore Database, modify `--singlestore` option.

src/LoggerUiServiceProvider.php:65

  • The ensureEnvExists() method is no longer called in boot(). Consider removing this unused method or reintroducing its guard logic to keep the codebase clean.
private function ensureEnvExists(): bool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants