Qualität von Extensions messbar machen

[BugBuster]

Gibts schon ne Alpha irgendwo zum testen vom ModuleSecurityChecker (wie er nun laut typolight-forge im vollem Namen sich nennt) ?

[xtra]

[...] Außerdem ging es eher darum bestimmte Core Klassen wie die Datenbank nicht ständig neu zu initialisieren.

Diese Klassen schytzen sich selbst bereits indem der constructor protected ist und nur per getInstance() die Instanz erfragt werden kann. Daher sind die vernachlaessigbar bzw. braucht man nicht checken, da sie nicht instanziert werden koennen.

[b2m]

Hi

Diese Klassen schytzen sich selbst bereits indem der constructor protected ist und nur per getInstance() die Instanz erfragt werden kann. Daher sind die vernachlaessigbar bzw. braucht man nicht checken, da sie nicht instanziert werden koennen.

das ist mir und jedem, der das Singleton-Pattern kennt durchaus klar (auch wenn ich mich weiter oben diesbezüglich etwas undeutlich ausgedrückt habe => wird korrigiert). Ich persönlich unterstelle Leo jedoch, dass er sich durchaus etwas dabei gedacht hat, als er bestimmte Klassen in der System an Attribute gebunden hat. Deshalb halte ich es für unnötig, diese selbst zu holen, wenn sich die Klasse, von der ich ableite schon darum kümmert.

Gruß b2m

[xtra]

Ich persönlich unterstelle Leo jedoch, dass er sich durchaus etwas dabei gedacht hat, als er bestimmte Klassen in der System an Attribute gebunden hat. Deshalb halte ich es für unnötig, diese selbst zu holen, wenn sich die Klasse, von der ich ableite schon darum kümmert.

Nun verstehe ich dich, du willst also, dass der scanner die Files durchgeht und...

class Foobar extends Frontend {

  public function foo()
  {
    $this->import('Input'); // ... hier lauthals jammert ...
  }
} 

...dass in Frontend Input schon da ist.

So etwas habe ich mir auch schon angedacht, muss jedoch warten bis lindesbs wieder da ist, damit ich mich mit ihm dementsprechend mal kurzschliessen kann.

[xs-hoppelhase]

Was auf jeden Fall Bestandteil des Security-Check-Moduls sein sollte (was mich immer Andreas Schempp gelehrt hat), ist auch auf "$this->Input->get('act')" zu achten, wenn man bestimmte Funktionen nicht nutzen will. Auch wenn man die Buttons ausblendet kommt man ja über die URL trotzdem heran.

Hier könnte man auch auf eine einheitliche Schreibweise der Funktion "tl_..._checkPermission" achten.

Beispiel:

// Check current action
switch ($this->Input->get('act'))
{            
    case 'paste':
        // allow
        break;
    case 'select':
        // allow
        break;
    case 'create':
        // allow
        break;
    case 'cut':
    case 'copy':
    case 'edit':
    case 'show':
    case 'delete':
    case 'editAll':
    case 'deleteAll':
    case 'overrideAll':
    case 'cutAll':
    case 'copyAll':                    
        if (!in_array($id, $root))
        {
             // $this->log('...','...', TL_ERROR);
             // $this->redirect('typolight/main.php?act=error');
        }
        break;
    default:
        // allow
        break;
} 

[lindesbs]

Wie siehts hier aus ? Habt ihr noch Interesse an diesem Projekt ?
Ideen sammeln war ja ganz nett. Aber sollte man es nciht auch mal angehen ?

[deerwood]

Moin Stefan,

ja, ich habe nach wie vor Interesse. Nun wollte ich mir gerade Dein(/Yanniks?, der als Entwickler ausgewiesen ist) MSC Tool herunterladen ... da ist aber nix (die Wiki-Seite ist da); auch im Extension-Repository konnte ich es nicht finden?

LG, Georg

[andreas.schempp]

Ich melde mich etwas spät, bin erst jetzt über das Thema gestolpert. Ich finde eine (Sicherheits-)Prüfung auch sehr wichtig, wir hatten das im Partner-Forum auch schon auf meine Anregung hin diskutiert.

Die Frage ist natürlich, wer die Kontrolle macht, und ob es etwas kosten darf. Ein "Premium-Siegel" könnte für professionelle Entwickler durchaus interessant sein, da es ja die Qualität Ihrer Arbeit zeigt. Aus meiner Sicht müsste JEDES Modul welches veröffentlicht wird kurz geprüft werden, insbesondere von neuen Accounts. Die Erweiterung wird dann allerdings nicht sofort freigeschaltet, was ein gewisses Hindernis sein kann.

[lindesbs]

Von diesem "Siegel" halte ich seit Anfang nichts. Ich sehe dieses Tool eher im Sinne als Hilfe fuer einen Entwickler, das er kontrollieren kann, ob alle notwendigen Dinge beruecksichtigt wurden.

Dein gewuenschtes "Siegel" koennte nur durch menschliche Kontrolle erfolgen. So wie wir es ind er Vergangenheit bei der ein oder anderen Erweiterung auch schon gemacht haben.

Aber automatisiert das Ganze, waere einfach schlecht.

Ich habe das Beispiel auch nur fuer mich geschrieben, um mal selbst darueber nachzudenken, was ich alles "falsch" mache oder vergesse.
Da sind stellenweise schon besondere Dinge, die vergessen werden, wie htaccess oder die die() Methoden am Anfang der Datei. Das muss aber vom menschlichen Geist begutachtet werden, weil Cron Module zum Beispiel dieses nicht haben duerfen. SOmit ist eine automatische Kontrolle hinfaellig.

[acenes]

Aus meiner Sicht müsste JEDES Modul welches veröffentlicht wird kurz geprüft werden, insbesondere von neuen Accounts. Die Erweiterung wird dann allerdings nicht sofort freigeschaltet, was ein gewisses Hindernis sein kann.

Gerade da liegt der Hase ja im Pfeffer. Die Frage stellt sich wer kompetent genug, beauftragt, neutral, diplomatisch und Willens ist eine solche Prüfung durchzuführen - und dann auch noch genügend freie Kapazität hat das jedes mal zeitnah zu erledigen damit die Wartefrist im erträglichen Rahmen bleibt. Durch eine Freiwilligenorganisation ist das nicht zu leisten, und ich bin mir sicher dass kein OS Anbieter noch dafür zahlen will dass seine Erweiterung überhaupt veröffentlicht wird.

Ein solches Prüftool ist da sicher nicht verkehrt, wobei der Output wie richtig angemerkt durch einen Menschen interpretiert und ggf. kommentiert werden muss, sonst muss das Tool etwas zahnlos bleiben weil es X Sonderfälle gibt wo von den Standardmechanismen halt doch mit Recht abgewichen werden kann.

Es wäre denkbar die Veröffentlichung im Repository auf den kommentierten Prüfbericht abzustützen, respektive diesen als obligatorische Eingabe zu verlangen welchen sich interessierte Personen anzeigen lassen können. Das alles gibt noch keine Garantien, aber es zwingt den Entwickler dazu sich Gedanken über die "Best Practises" für TL vertraut zu machen. Die Probleme orte ich nämlich zum überwiegenden Teil bei neu-Entwicklern welche mit dem TL Framework noch nicht vertraut sind.

[lindesbs]

Es wuerde ja ausreichen, wenn ein "Level" angezeigt werden wuerde.
Im Sinne des DiagnoseTools fuer TL. Einfach ein DiagnoseTool fuer Erweiterungen.
Und im ER kann man sich dann zu jeder Erweiterung das Pruefprotokoll anzeigen lassen.

Also ein Diagnosetool im ER und eins fuer den Entwickler. zum eigenstaendigen Kontrollieren.

[andreas.schempp]

Nun in erster Linie sollten Erweiterungen von neuen Entwicklern geprüft werden. Wenn jemand schon 10 Erweiterungen veröffentlich hat, die gut sind, sollte das eigentlich als primäre Prüfung reichen. Oft machen die Anfänger halt die schlimmen Fehler wie die Input-Klasse nicht zu verwenden.

Ich habe bereits im Partner-Thread schon angesprochen dass ich mir grundsätzlich eine entsprechende Unterstützung vorstellen kann. Ich bin immerhin nicht als Moderator oder sonst was in der Community aktiv, also könnte ich mich da einbringen.
Ich frage mich mehr, wie wir mit Erweiterungen verfahren, die nicht akzeptiert werden. Ein einfaches "nein" reicht da ja nicht (was einfach wäre), sondern theoretisch müssen man dem Entwickler die richtige Lösung zeigen, und DA wird es sehr aufwändig.

[FloB]

Meines Erachtens sollte man, anstatt ein "Gütesiegel" für Extensions einzuführen, ein paar Punkte angeben, die für Extensions überprüft werden sollten. Besitzt die Erweiterung z. B. Standardstrukturen, wird dieser Punkt als erfüllt angesehen. Dadurch kann man als Nutzer und Entwickler besser erkennen, wo genau das Problem liegt. Eine optionale ausführliche Bewertung / Beschreibung / Meinung des Reviewers bringt dann in manche Punkte nochmal Klarheit.

Folgende Punkte fallen mir spontan ein:

1. Datei-Grundstruktur wird eingehalten (if-TL_ROOT-die, Lizenzerklärung, Kurzbeschreibung, Klasse)
2. Nutzt das Framework (wo zutreffend)
3. Einfache Sicherheit: Input wird gesäubert, DB-Queries prepare't, etc.
4. Datenschutz: Bestimmte Daten werden verschlüsselt UND es werden keine Daten an Dritte gesendet ODER die Datenschutzbestimmungen werden beim Datenversand eingehalten (Nutzer informieren etc.)
5. …

Alle Punkte gelten natürlich nur dort, wo sie Sinn machen. Andreas' [ajax]-Erweiterung würde z. B. Punkt 1 passieren, obwohl der if-TL_ROOT-die-Check in der einzigen Datei dieser Erweiterung fehlt – aber hier macht sie auch keinen Sinn.
Ich muss auch bei einer (privaten) Erweiterung auf das $_POST-Array zurückgreifen, da (bis 2.8) keine Arrays in Input::post() erkannt werden – ich prüfe aber den String dann selber. Somit wären Punkt 2 (Framework kann ich hier nicht nutzen, da mir sonst Funktionalität verloren gehen würde) und 3 ebenfalls erfüllt.

Klar, bei einem "Nein" steht der Entwickler erstmal blöd da. Aber dafür gibt es ja Dokumentation (und Forum), und zusammen mit ein paar Hinweisen des Reviewers (nutzt Framework nicht, potentielle Schwachstelle durch SQL-Injection … in Zeilen a-b / überall) sollte der Entwickler dann in der Lage sein, das Problem zu lösen.

[andreas.schempp]

Ich fände einen Kriterienkatalog gut. Möglichst ausführlich. Dann verweist man den Entwickler dorthin, und sagt "Punkt 5 ist nicht erfüllt".

[xtra]

Ich frage mich mehr, wie wir mit Erweiterungen verfahren, die nicht akzeptiert werden. Ein einfaches "nein" reicht da ja nicht (was einfach wäre), sondern theoretisch müssen man dem Entwickler die richtige Lösung zeigen, und DA wird es sehr aufwändig.

Diese Erweiterungen haben einen geringen "autolevel", mehr passiert aber auch erstmal nicht.

Man kann auf jeden Fall aber beim "Entwicklercheck" Hinweise geben wie z.B. "consider using $this->Input->get() instead of $_GET".

Evtl. koennte man auch im Code eine spezielle Art von Kommentaren folgender Art einbauen:

// Wir brauchen hier kein $this->Input()
// TL_CODE_CHECK['OFF']
if(count($_GET))
{
// Ab hier wieder checken
// TL_CODE_CHECK['ON']
[...]
} 

Aehnlich gehen auch andere "Compiler" wie VC++, Delphi, Free Pascal usw. vor.
Der Entwickler bekommt also Hinweise, welche er, nachdem er seinen Code validiert hat, gezielt deaktivieren kann.
Die Lesbarkeit leidet IMO nicht sonderlich darunter.

Logischerweise sollten dann nicht die Entwickler dazu ueber gehen am Anfang und am Ende einer jeden Datei den check zu disablen, aber eine Grundidee ist es denke ich.

[andreas.schempp]

Ich bin immer noch für ein manuelles Reviewing. Theoretisch könnte ich auch eine Erweiterung online stellen, welche mir alle DB- und FTP-Zugangsdaten per E-Mail zusendet und einen Backend-User erstellt (der unsichtbar ist)...

[webstar]

Über die Überprüfung von bestehenden Erweiterungen habe ich desöfteren nachgedacht. Generell finde ich, dass es für alle Erweiterungen gut ist, einer Überprüfung zu unterliegen. Selbst Entwickler, mit 20 Erweiterungen sind Menschen und machen hier und da mal einen Fehler.

Ein Tool, was Sachen automatisch prüft, wäre für mich ein erster Schritt zur Überprüfung. Dann ist die Frage, wie man damit umgeht. Meiner Meinung nach wäre es erstrebenswert im ER eine Rückmeldung zu bekommen, inwieweit ein Modul überprüft worden ist. Aber dafür eignet sich imho eine maschinelle Überprüfung nicht. Sie könnte vielmehr als Grundlage für eine menschliche Überprüfung dienen, die dann gewissen Punkte nochmals überprüft.

Ein Beispiel wäre der Aspekt der Datensicherheit. Eine Erweiterung darf bzw. muss ja teilweise mit externen Diensten kommunizieren, wie z.B. newstwitter oder meine Dropbox-Erweiterung. Aber alle anderen Verbindungen sollten ausgeschlossen sein. Dies kann aber letztlich nur ein Mensch beurteilen, inwieweit hier noch andere Verbindungen möglich sind.

Ich weiß, dass eine Überprüfung der Extensions ein großer Aufwand ist. Aber gerade bei einen Shop oder irgendwelchen Bezahldiensten wäre ein gewisses Siegel denke ich für jeden hilfreich. Die Frage ist, ob man so etwas als Dienstleistung anbieten könnte. Dass im Rahmen des TYPOlight-Teams/ Partner es Verantwortliche gibt, die eine Überprüfung durchführen können und man sich als Nutzer quasi eine Lizenz zur Überprüfung erwerben kann.

Inwieweit so etwas auch kostenlos mit der Community möglich wäre, weiß ich nicht. Aber selbst wenn es eine Dienstleistung wäre, würde die gesamte Community profitieren, da letztlich Erweiterungen verbessert werden.

Dies sind nur ein paar Gedanken, die mir grundlegend dazu kommen.

[xtra]

Ich bin immer noch für ein manuelles Reviewing. Theoretisch könnte ich auch eine Erweiterung online stellen, welche mir alle DB- und FTP-Zugangsdaten per E-Mail zusendet und einen Backend-User erstellt (der unsichtbar ist)...

Klar, das kann jeder und mal Hand auf's Herz, wer checkt wirklich jede Extension durch, die er aus dem ER zieht.
Ok, 90% der von mir benutzten kenne ich, da ich sie meist leicht modifiziere, aber allgemein von den "Normal-Usern" (bitte wertfrei auffassen) gesehen denke ich, dass die das blauaeugig installieren.

[andreas.schempp]

Wie wäre es denn einfach mit einem "Überprüft durch Community" Gütesiegel? Das sollte wenn möglich jede Erweiterung erhalten, das wäre das Ziel. Eine Erweiterung welche es noch nicht hat, sollte ggf. mit Vorsicht beachtet werden. Die Erweiterung kann trotzdem im Repository verfügbar sein, und innerhalb nützlicher Frist sollte jemand vom Reviewing-Team das dann beurteilen.
Wenn eine Erweiterung als schlecht befunden wird, müsste man sie dann aus dem Repository entfernen (bzw. unveröffentlichen) können.

[lucina]

Naja - habe wir sowas nicht schon mit 'bewerten'? Und wenn ich das so anschaue, dann scheint es für mich (wie bei heise auch) durchaus üblich, dass sich der Ersteller / die Erstellerin einer Extension selbst erst einmal die volle Punktzahl gibt.

Was das verbessern könnte: Beispielsweise im App-Store von Apple werde ich zumindest beim Deinstallieren darauf hingewiesen, die App zu bewerten - so etwas in der Art könnte man auch einbauen, womöglich in den ER-Client und versionsbezogen.

[FloB]

Was das verbessern könnte: Beispielsweise im App-Store von Apple werde ich zumindest beim Deinstallieren darauf hingewiesen, die App zu bewerten - so etwas in der Art könnte man auch einbauen, womöglich in den ER-Client und versionsbezogen.

Wie oft hast du schon Erweiterungen im ER-Client deinstalliert?

[lucina]

Glaube mir: öfter als mir lieb war. Aber natürlich hast Du Recht, das sollte nicht an die Deinstallation gekoppelt sein. Die "5-Sterne-Kandidaten" sollten ja auch eine Chance bekommen.

Ich wollte auch eigentlich nur darauf hinweisen, dass die Möglichkeit zu einem Userrating bereits besteht.

[webstar]

Wie wäre es denn einfach mit einem "Überprüft durch Community" Gütesiegel? Das sollte wenn möglich jede Erweiterung erhalten, das wäre das Ziel. Eine Erweiterung welche es noch nicht hat, sollte ggf. mit Vorsicht beachtet werden. Die Erweiterung kann trotzdem im Repository verfügbar sein, und innerhalb nützlicher Frist sollte jemand vom Reviewing-Team das dann beurteilen.
Wenn eine Erweiterung als schlecht befunden wird, müsste man sie dann aus dem Repository entfernen (bzw. unveröffentlichen) können.

Ich könnte mir eine Art Level System vorstellen. Erweiterungen, die nicht geprüft worden, sind auf einem neutralen Level. Bei der Überprüfung können dann Bewertungen gegeben werden nach einem definierten Kriterienkatalog. Einzelne Punkte könnten gesondert gewertet werden. Aspekte der Sicherheit wiegen für mich zum Beispiel mehr als jetzt eine Abweichung vom TL-Framework. Und dann könnte beim Level eine Erklärung dabeistehen, wie Level -3 da fehlende Überprüfung von Input-Werten.

Somit gäbe es eine Überprüfung durch ein Team, der Endanwender hat jedoch selbst die Wahl sich Erweiterungen zu installieren. So könnte man sie z.B. leicht selbst modifizieren, dass Erweiterungen auch sicher funktionieren.

Die entscheidende Frage ist dann, wer in so einem Team arbeitet. Denn die Möglichkeit für Reviews gibt es ja bereits jetzt und wird nur sporadisch verwendet.

Eine Frage, die sich mir dann auch noch stellt, ist ab wann eine Erweiterung geprüft wird. Erweiterungen, die in der Entwicklungsphase sind - und für Testzwecke vielleicht trotzdem im ER stehen - wäre eine Überprüfung ein großer Aufwand. Daher wäre eine Richtlinie für die Versionierung sinnvoll. (z.B. Überprüfung ab RC, stable nur nach Überprüfung möglich).

Und wie wird mit Aktualisierungen von Erweiterungen umgegangen? Jedes Build erfordert theoretisch eine neue Überprüfung. Ein Tool, was Unterschiede anzeigt, wäre hier unerlässlich.

[andreas.schempp]

@Siegel: Ich bin dagegen so was zu vergeben. Denn wenn man sagt BSP: Schempp, Feyer, Koch, ... dürfen ein Siegel vergeben und bekommen prü Check 100 €. Was machen dann die Leute die das Geld nicht haben? Sind diese Extensions dann automatisch schlechter?

Ich meinte unbezahlt. Und wenn möglich sollte alles bewertet werden. Entweder
- schlecht gemacht: fällt raus
- nicht bewertet: noch niemand hatte Zeit
- bewertet: für gut befunden

Für gut befinden bedeutet, es bestehen keine offensichtlichen Sicherheitsrisiken (Input-Klasse usw). Weitere Verbesserungshinweise können direkt an den Entwickler gehen und müssen gar nicht öffentlich sein.

Und wie wird mit Aktualisierungen von Erweiterungen umgegangen? Jedes Build erfordert theoretisch eine neue Überprüfung. Ein Tool, was Unterschiede anzeigt, wäre hier unerlässlich.

Das Tools ist ja wohl das kleinste Problem und um die technische Natur sollten wir uns noch nicht kümmern. Interessant wäre, wie viele Erweiterungen denn pro Woche veröffentlicht werden.

[FloB]

Und wie wird mit Aktualisierungen von Erweiterungen umgegangen? Jedes Build erfordert theoretisch eine neue Überprüfung. Ein Tool, was Unterschiede anzeigt, wäre hier unerlässlich.

Eine Überprüfung pro Build und Maintenance-Version ist nicht praktikabel. Ich würde eine Prüfung pro Minor-Version vorschlagen, wobei das optional ist und bei einem Siegel "geprüft mit a.b.c" angezeigt werden kann, wenn die Prüfung nicht der aktuelle Version entspricht. Bei Majors fällt das Siegel weg, und wird erst nach erneuter Überprüfung angezeigt.

Bleibt aber ein Problem: Die Entwickler halten sich nicht an die empfohlene Versionsnummerierung. Somit ist es schwer zu sagen, was jetzt Major-, Minor- und Maintenance-Release ist.

[andreas.schempp]

Bleibt aber ein Problem: Die Entwickler halten sich nicht an die empfohlene Versionsnummerierung. Somit ist es schwer zu sagen, was jetzt Major-, Minor- und Maintenance-Release ist.

Naja, wenn es um Sicherheit geht, muss auch jeder Build geprüft werden. Jeder Build kann Code-Änderungen haben, die geprüft werden müssen... Ein effizientes Review-System ist also unerlässlich, ein diff für jeden Build wäre aber aktuell gar nicht machbar...

[andreas.schempp]

Was nützen Tema-Vorschläge, wenn wir uns noch nicht einige sind was gemacht wird?
Du hast es schon wieder "gesagt", deshalb erwähn ich es nochmals: Ich spreche nicht von einem Rating, sondern von einer Prüfung, ob die Erweiterung veröffentlicht werden darf. Da wir aber nicht jeder VOR der Veröffentlichung prüfen können, würde ich das DANACH machen, und entsprechend kennzeichnen (oder löschen lassen).

[andreas.schempp]

Ja es darf auf keinen Fall etwas aus dem .de-Forum sein, sondern ein "offizielles Extension-Review-Team". Wer es am Ende macht ist eigentlich sekundär, wichtiger ist dass der Kriterienkatalog klar ist...

Vielleicht hat jemand vom TL-Team auch etwas dazu zu sagen?

[andreas.schempp]

Das sehe ich anders, da es sich um einen emensen Aufwand handelt ist das nicht sooo sekundär wie man glaubt. Du darfst nicht erwarten dass alle Programmierer einen schön lesbaren Stil drauf haben, was manche Leute da coden kommt PERL schon sehr nahe, sprich du brauchst noch viel länger um alles zu lesen und zu testet.

das stimmt natürlich, aber die primäre Frage ist was wir wollen, und danach sehen wir wie und ob es machtbar ist... so ist mein Vorgehen

[andreas.schempp]

Finde ich eine sehr gute Idee, das am Usertreffen zu diskutieren.

[Nina]

Ich denke auch, dass sowas ideal abends beim Usertreffen diskutiert wird. Wir haben schon bei den letzten Usertreffen gesehen, dass dieser lockere Rahmen oft zu sehr guten Ideen führt und vor allem die direkte Kommunikation etwaige Missverständnisse etwas unwahrscheinlicher macht.

Dennoch schadet es ja nicht, wenn ihr die Thematik vorab hier im Thread besprecht und Ideen sammelt. So können sich davor auch schon die beteiligen, die nicht beim Usertreffen sein werden.

[FloB]

Pro Usertreffen, auch wenn ich nicht dabei bin. Man kann ja schonmal Brainstorming machen, bezüglich Vorgehensweise und Themen-, Prüfkatalog.

Wir brauchen eigentlich dringend ein Wiki