Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logs de SimplePie #711

Closed
plopoyop opened this issue Nov 24, 2014 · 14 comments
Closed

Logs de SimplePie #711

plopoyop opened this issue Nov 24, 2014 · 14 comments

Comments

@plopoyop
Copy link

Salut,
Il semble qu'il y ait des logs un peu trop verbeux de la part de SimplePie dans /var/log/message
(je n'utilise pas de flux nécessitant une authentification mais) un utilisateur du package yunohost me remonte qu'on trouve les identifiants / mot de passe des fluxs :
Nov 24 16:10:02 machinename php: SimplePie GET http://user:PASSWORD@tempsreel.nouvelobs.com/rss.xml

Je pensais ouvrir l'issue directement sur le github de SimplePie, vu que c'est la librairie qui log.
Mais je suis tombé sur la ligne :
syslog(LOG_INFO, 'SimplePie GET ' . $url); //FreshRSS

Donc j'imagine que ça vient d'une évol' faite par FreshRSS.

Est-il possible d'annonymiser l'url lors du log ?

CF :
https://github.com/plopoyop/freshrss_ynh/issues/4

@aledeg
Copy link
Member

aledeg commented Nov 24, 2014

Il faut qu'on regarde ça. Merci

@aledeg
Copy link
Member

aledeg commented Nov 26, 2014

La ligne incriminée est dans le fichier lib/SimplePie/SimplePie/File.php à la ligne 82. J'ai comparé avec le code source de SimplePie et il semble que ce soit un ajout de FreshRSS.
À mon avis, il faut supprimer cet appel ou trouver un moyen d'anonymiser la connexion.

@Alkarex
Copy link
Member

Alkarex commented Nov 26, 2014

C'est moi qui ai ajouté cette ligne, car SimplePie ne loggue pas son activité. Je vais m'en occuper.

@Alkarex Alkarex self-assigned this Nov 26, 2014
@aledeg
Copy link
Member

aledeg commented Nov 27, 2014

Ce n'est pas le seul endroit où il faut enlever les identifiants. Voici les différents endroits que j'ai pu trouver :

  • app/Models/Feed.php ligne 251
  • app/Models/Feed.php ligne 254
  • lib/lib_rss.php ligne 171
  • lib/SimplePie/SimplePie.php ligne 1532
  • lib/SimplePie/SimplePie.php ligne 1536
  • lib/SimplePie/SimplePie/File.php ligne 82

@ZeHiro
Copy link

ZeHiro commented Dec 16, 2014

Bonjour,
le log de l'api (dans data/log/api.log) est aussi très verbeux. On retrouve le couple passwd/mdp en clair.

@marienfressinaud
Copy link
Member

@Alkarex > si tu n'as pas le temps de t'occuper de ce ticket cette semaine je le ferai, j'aimerais clore ce ticket avant la 1.0.

@Saruspete
Copy link

Hello,

Pouvez vous ajouter une fonctionnalité pour desactiver les syslogs ?
Ou en quickfix, ajouter une directive "openlog()" qui permettrait de selectionner la factory a utiliser (et ainsi de dropper les messages) ?

Merci à tous !

@marienfressinaud
Copy link
Member

Oui d'ailleurs @Alkarex je ne suis pas fan d'utiliser les syslogs dans le code de FreshRSS : il y a déjà Minz_Log pour faire le travail de logguer les erreurs (et qui mériterait certes d'être amélioré). Ou alors il faudrait établir des règles pour quand utiliser syslog et quand utiliser Minz_Log. Actuellement seuls les logs que tu as placé dans SimplePie me semble pertinents puisqu'il ne faut pas mélanger Minz à SimplePie.

Donc ce que je compte faire :

  • Remplacer les appels à syslog par des appels à Minz_Log::notice(). À la limite je précise un fichier de log différent ?
  • Écrire une fonction dans lib_rss permettant de supprimer les identifiants des URL
  • Utiliser cette fonction aux endroits relevés par @aledeg

Par contre je suis embêté pour les logs dans SimplePie, je ne sais pas trop quoi en faire notamment parce que je ne pourrai pas utiliser la fonction de lib_rss ni Minz_Log… À la limite je les commente mais ça pourrait être embêtant pour débugguer…

@aledeg
Copy link
Member

aledeg commented Jan 27, 2015

Il faudrait peut-être faire une interface à SimplePie dans FreshRSS ou étendre la classe et redéfinir certaines méthodes.
De cette manière, on ne touche pas à SimplePie (plus facile à migrer) et on a toutes les modifications au même endroit.

@Alkarex
Copy link
Member

Alkarex commented Jan 27, 2015

Je pourrais par exemple faire une fonction dans SimplePie pour activer/désactiver ces logs.

marienfressinaud added a commit that referenced this issue Jan 29, 2015
Temporary fix:

- Change syslog by Minz_Log::notice in most of the files
- Logs are stored in USERS_PATH/_/log.txt for actualize_script.php
- Simply comment syslog in SimplePie

See #711
marienfressinaud added a commit that referenced this issue Jan 29, 2015
marienfressinaud added a commit that referenced this issue Jan 29, 2015
Use @aledeg old function instead

See #715
See #711
@marienfressinaud
Copy link
Member

Ce que j'ai fait afin de corriger le soucis dans la 1.0 :

  • Remplacement de syslog par Minz_Log::notice. Dans SimplePie j'ai simplement commenté les lignes en attendant mieux.
  • J'ai créé une fonction url_remove_credentials dans lib_rss.php à utiliser à chaque fois qu'on a besoin d'une url sans les identifiants (principalement dans les logs mais aussi pour l'attribut Feed->url). J'ai réutilisé le fonctionnement de Add a function to prepare URL for logging #715 (je l'avais oublié au début :p).
  • Je n'ai pas ajouté url_remove_credentials dans SimplePie pour ne pas le lier à FRSS. Comme les lignes sont commentées ça ne pose pas de problème mais du coup on ne bénéficie plus des lignes ajoutées par @Alkarex !

La question qu'il me reste : que fait-on pour les logs de l'API greader.php ? Ils sont effectivement très verbeux… une solution pourrait de ne logguer que si l'environnement est mis à development. Au moins pour le moment, non ? Je me rends compte que le fait de n'avoir que development et production pour gérer le niveau de log est quand même très juste…

Alkarex added a commit to Alkarex/FreshRSS that referenced this issue Mar 21, 2015
Alkarex added a commit to Alkarex/FreshRSS that referenced this issue Mar 22, 2015
@Alkarex
Copy link
Member

Alkarex commented Mar 22, 2015

Voir #815
Commentaires bienvenus.

Alkarex added a commit to Alkarex/FreshRSS that referenced this issue Mar 24, 2015
@ZeHiro
Copy link

ZeHiro commented Mar 25, 2015

Ça m'a l'air bon pour le log de l'API

@Alkarex
Copy link
Member

Alkarex commented May 10, 2015

Corrected in /dev branch #815
There is now a simplepie_syslog_enabled boolean parameter in config.php, to switch on/off syslog in SimplePie. In any case, credentials are removed.

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

No branches or pull requests

6 participants