http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268
--- Comment #15 from Jared Camins-Esakov <[email protected]> --- (In reply to comment #12) > (En réponse au commentaire 11) > > > > Marking passed QA, I'll delay pushing, I'll wait for some feedback from > > semarie, just in case > > The code seems globally ok: just 4 remarks. > > Some security remarks (in tools/export.pl): > 1. a problem (common in koha): use of user-input in warn. An attacker can > forge entries in log file (see CWE-93) (resume: if variable contains CRLF, > the log file too... and the next line is forged: so who write in log ? koha > or attacker ?) > > > warn "A suspicious attempt was made to download the db at '$filename' by > > ..." > > > 2. about the regex for $filename verification > > return unless ( ... && not $filename =~ m#(^\.\.|/)# ); > > The regex could be translated as: $filename should not: > - start with '..' > OR > - contains '/' > > I don't understand the purpose of "^..", if '/' is forbidden; as you can't > escape from directory without '/'. > > So I think m#/# is suffisent (please correct me if not). > > > > Some generals remarks: > 3. the directory for backup is not consitent in differents files: > > in debian/templates/koha-conf-site.xml.in, the directory use for backupdir > is: > > <backupdir>/var/lib/koha/__KOHASITE__</backupdir> > > in Makefile.PL, the directory seems to be in /var/spool : > > './skel/var/spool/koha' => { target => 'BACKUP_DIR', trimdir => -1 }, > > in debian/scripts/koha-dump: > > [ -z "$backupdir" ] && backupdir="/var/spool/koha/$name" > > > 4. in misc/cronjobs/backup.sh > > The lasts lines are for mail admin if ok, or mail admin if not ok. > > [ -f $KOHA_BACKUP] && echo "$KOHA_BACKUP was successfully created." | mail > > kohaadmin -s $KOHA_BACKUP || > > echo "$KOHA_BACKUP was NOT successfully created." | mail kohaadmin -s > > $KOHA_BACKUP > > First, there are *two* lines. I haven't test it, but it should be an error > as "cmd_a && cmd_b ||" is not a valid shell expression. Should be one-line > "cmd_a && cmd_b || cmd_c" > > Secondly, I'm not sure that the behaviour is correct. > "cmd_a && cmd_b || cmd_c" could be expanded as: > > if cmd_a ; then > if ! cmd_b ; then > cmd_c > fi > else > cmd_c > fi > > so if cmd_b failed (echo "backup ok" | mail admin), the cmd_c (echo "backup > err" | mail admin) will be run (and expected to failed too, as the first > try). > > Proposed correction: > > if [ -f $KOHA_BACKUP] ; then > > echo "$KOHA_BACKUP was successfully created." | mail kohaadmin -s > > $KOHA_BACKUP > > else > > echo "$KOHA_BACKUP was NOT successfully created." | mail kohaadmin -s > > $KOHA_BACKUP > > fi Thank you for catching those issues. I corrected them all with the follow-up. (In reply to comment #11) > QA comment: > > You add 2 subs: getbackupfilelist and download_backup => bad point, we > usually use capitals (GetBackupFileList and DownloadBackup). However, this > is not in a package, so I won't fail QA for that. Our standard is to use lowercase in .pl files and uppercase in packages, I think. I suppose that makes sense, since it ensures that there will be a visual distinction between local functions and core functions. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
