FileCloud – code review

Cześć, mógłbym prosić o code review w celu ewentualnej poprawy, dowiedzenia się, co można zrobić lepiej? Aplikacja jest "choć trochę" opisana w README.md, mam w liście TODO zapisane, by niedługo napisać bardziej kompleksowe README.md z opisami i screenami.

Link do repozytorium: https://github.com/Jockiee/FileCloud

Stack technologiczny: na frontendzie wiadomo HTML, CSS, Bootstrap 4 oraz jQuery, na backendzie Java, Spring, PostgreSQL, JPA.

Moim celem było wykonać tą aplikację jako SPA. Na początku była to typowa dynamiczna aplikacja webowa, ale dokonałem refaktoryzacji do SPA.

Z góry dziękuję i pozdrawiam, gjm

Pierwsza czerwona flaga:

Moim celem było wykonać tą aplikację jako SPA. Na początku była to typowa dynamiczna aplikacja webowa, ale dokonałem refaktoryzacji do SPA.

Dobre powody do użycia SPA: zwiększenie responsywności, model cienkiego serwera, redukcja ilości zduplikowanej logiki, team mający większe doświadczenie z frontendem niż z backendem, miłośc do js.

Złe powody do użycia SPA: "celem było wykonać tą aplikację jako SPA".

Wiem że to pewnie zabawkowy projekt, ale normalnie użyte technologie powinny być uzasadnione przez biznesowe potrzeby ;). Patrząc na niektóre fragmenty, mam duże wątpliwości czy to była refaktoryzacja.

Ok, do kodu:

spring.datasource.url=jdbc:postgresql://localhost:5432/file_cloud
spring.datasource.username=postgres
spring.datasource.password=123456

Mam nadzieję że nie trzymasz hasła w kontroli wersji (nawet mimo że baza jest lokalna). Hasło 123456 to tez średni pomysł :P.

Plik: https://github.com/Jockiee/FileCloud/blob/master/src/main/resources/templates/index.html - kompletnie losowe formatowanie kodu. Weź go chociaż przepuść przez jakiś formatter HTML.

No i niby masz styles.css, ale kod pełny inline stylów w rodzaju <div style="height:0%; position:absolute; bottom:50px; right: 0px;">.

Plik: https://github.com/Jockiee/FileCloud/blob/master/src/main/resources/static/js/spa.js - hmm, własny framework do SPA?

https://github.com/Jockiee/FileCloud/blob/master/src/main/resources/static/js/spa.js#L120 - ratunku.

' <button type="button" data-filename="' + fileName + '" class="btnDelete btn-danger" style="position: absolute; right: 5px; top: 5px;">X</button></li>';

Js z inline HTML z inline CSS. No i ten, fileName brzmi jak coś kontrolowanego przez użytkownika, czyli piękny XSS. https://en.wikipedia.org/wiki/Cross-site_scripting

To tyle z HTML/JS - części javowej nie przeglądałem.

1. Twórcy mockito ucieszą sie ze kolejny raz ktoś przetestował ich bibliotekę. Bo własnego kodu nie testujesz tam w ogóle
2. https://github.com/Jockiee/FileCloud/blob/master/src/main/java/com/gjm/file_cloud/entity/File.java @jarekr000000 się ucieszy z takiego annotation-driven-development :D

try {
	 fileToAdd = new File();
	 fileToAdd.setName(file.getOriginalFilename());
	 fileToAdd.setType(file.getContentType());
	 fileToAdd.setBytes(file.getBytes());
 } catch(IOException exc) {
	 exc.printStackTrace();
 }
 fileService.addFile(fileToAdd);
 return ResponseEntity
		 .status(HttpStatus.CREATED)
		 .body(file.getOriginalFilename());

Faktycznie, nie ważne że coś sie wywaliło, i tak zrobimy add file i powiemy userowi że created ;)
4. Tutaj wisienka na torcie: https://github.com/Jockiee/FileCloud/blob/master/src/main/java/com/gjm/file_cloud/service/FileServiceHardDiskImpl.java#L28 Polecam sprawdzić co się dzieje jak zrobisz: new File("alamakota/sierotkamarysia/../../test.txt").createNewFile();. A teraz wyobraź sobie ze ktoś uploaduje ci tam coś ciekawego, np. plik ../../../home/username/.profile albo podmienia jakiś inny plik systemowy ;)

@msm:

Jeśli chodzi o SPA to jak robiłem wcześniej różne aplikacje webowe bez architektury SPA, to wychodziły małe potwory; tzn. używałem szablonów Thymeleaf, czyli server-side rendering i nie miałem pomysłu na LOGICZNĄ organizację takiej appki --> jakieś rady, czy kwestia doświadczenia?

Jeśli chodzi o dodanie application.properties do .gitignore to szczerze muszę się przyznać, że zapomniałem to zrobić, już się poprawiam, wiem że tu masz oczywiście rację.

Jeśli chodzi o "Plik: https://github.com/Jockiee/Fi[...]in/resources/static/js/spa.js - hmm, własny framework do SPA?", to nie miałem pomysłu jak nazwać lepiej ten plik - główny plik aplikacji SPA; app.js/spa.js/script.js/jakieś inne propozycje? Używałem jQuery, gdyż z frontendem dopiero zaczynam i React/Vue jest dopiero w moich planach, na szczęście niedalekich.

Jeśli chodzi o XSS, to metodą zapobiegania w tym przypadku będzie dodanie pustego buttona i używając metod DOM'a/jQuery dodanie np. klas i atrybutów metodami typu addClass()/attr(), zamiast ręcznie wpisywanie i konkatenowanie stringa, gdyż jQuery waliduje nam te atrybuty, dobrze mówię? Nie mam za bardzo pomysłu zrobić to tutaj inaczej.

@Shalom:

3. Już poprawiam, tu masz rację.
4. Wiem o co Tobie tutaj chodzi, ale masz jakiś pomysł jak mogę to walidować? Mój jedyny pomysł to sprawdzać przekazany string regexpami w celu odrzucania znaków typu .. <- cofnięcie do poprzedniego katalogu.

Dzięki za wszystkie odpowiedzi!
Pozdrawiam, gjm

Jeśli chodzi o XSS, to metodą zapobiegania w tym przypadku będzie dodanie pustego buttona i używając metod DOM'a/jQuery dodanie np. klas i atrybutów metodami typu addClass()/attr(), zamiast ręcznie wpisywanie i konkatenowanie stringa, gdyż jQuery waliduje nam te atrybuty, dobrze mówię? Nie mam za bardzo pomysłu zrobić to tutaj inaczej.

Nie waliduje wystarczająco - ważny jest kontekst (zobacz niżej). Generalnie jQuery nie nadaje się do budowania DOMa w sposób bezpieczny. Za dużo pułapek i każda linijka (jQuery) to potencjalna mina.
https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet

W zasadzie musisz użyć jakiegoś enginu templatowego: Angular, React JSX/TSX, żeby to było bezpieczne.

W zasadzie musisz użyć jakiegoś enginu templatowego: Angular, React JSX/TSX, żeby to było bezpieczne.

@jarekr000000 żebyś sie nie zdziwił :D https://www.blackhat.com/docs/us-17/thursday/us-17-Lekies-Dont-Trust-The-DOM-Bypassing-XSS-Mitigations-Via-Script-Gadgets.pdf z blackhata :) Tylko na Reacta nie znaleźli podatności.

Wiem o co Tobie tutaj chodzi, ale masz jakiś pomysł jak mogę to walidować? Mój jedyny pomysł to sprawdzać przekazany string regexpami w celu odrzucania znaków typu .. <- cofnięcie do poprzedniego katalogu.

Nie nie nie. To najgorsze co możesz zrobić. Na bank będzie do obejscia. Najpierw znormalizuj ścieżkę (np. jakimś FilenameUtils:normalize albo Path:getRealPath) a potem sprawdź czy faktycznie leży w tym bazowym katalogu.

Hey, poprawiłem to co mogłem. W liście commitów dostępna jest historia poprawek => 1 poprawka = 1 commit.

Macie może jeszcze jakieś sugestie?

Pozdrawiam, gjm

if(!pathIsCorrect(rootDirectory + name)) {
    throw new IllegalArgumentException("Final path isn't placed in root directory! Trying to do LFI? :)");
}

Zamiast copy-paste wydziel może po prostu funkcje validatePath()? ;)
2. Logika w kontrolerze dla addFile - nie powinno jej tam być. Czemu kontroler ma mapować tego input beana na jakaś klasę entity? Czemu nie robi tego warstwa logiki (czyli np. serwis)? Co więcej konstruktor albo builder zawsze są lepsze od takiej zabawy seterami.

@Shalom: Dzięki za odpowiedź!

1. Rozumiem, że jeśli metoda validatePath() rzuci nam dany wyjątek, to serwis jego nie łapie tylko ma zadeklarowanego w throws, tak?
2. Ok, już to robię, nie pomyślałem o tym.

Pozdrawiam, gjm