[Koha-bugs] [Bug 8268] Koha should offer way to backup entire db

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8268

--- Comment #15 from Jared Camins-Esakov <jcamins at cpbibliography.com> ---
(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.