From d3ebe2e371c97eafa68d6f17b923b2fc654bc01a Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Mon, 8 Apr 2024 19:06:40 +0200 Subject: [PATCH 01/11] Tests: expect a validation error when a theme has multiple XMLs --- constants/base.py | 1 + .../tests/files/theme-more-than-one-xml.zip | Bin 0 -> 6068 bytes extensions/tests/test_submit.py | 14 +++++++ files/forms.py | 29 +++++++++------ files/utils.py | 35 +++++++++++++++--- 5 files changed, 62 insertions(+), 17 deletions(-) create mode 100644 extensions/tests/files/theme-more-than-one-xml.zip diff --git a/constants/base.py b/constants/base.py index f8aeb088..251a3120 100644 --- a/constants/base.py +++ b/constants/base.py @@ -58,6 +58,7 @@ EXTENSION_TYPE_PLURAL = { EXTENSION_TYPE_CHOICES.THEME: _('Themes'), } EXTENSION_SLUGS_PATH = '|'.join(EXTENSION_TYPE_SLUGS.values()) +EXTENSION_TYPES = {v: k for k, v in EXTENSION_TYPE_SLUGS_SINGULAR.items()} ALLOWED_EXTENSION_MIMETYPES = ('application/zip', ) # FIXME: this controls the initial widget rendered server-side, and server-side validation diff --git a/extensions/tests/files/theme-more-than-one-xml.zip b/extensions/tests/files/theme-more-than-one-xml.zip new file mode 100644 index 0000000000000000000000000000000000000000..392e518f2a0a1a79777348a59e47c873ddd1cf87 GIT binary patch literal 6068 zcmZ{oWl)^Ux`hXaL4!jEclSVo1PSi$F2M$OcZU!xK!8C8clQZSfIx6}4-!JqpqHGz z?>_tPbI;fHc6C>My;i@gs{cGH@(74P!0!*qB}(UCi$5o100`h_Z3(vI(9}Q!AOP3# zte!_sS-N|n1K<$9Apih~2TzmA(|P=-6Q}@>WL+K2Zd%Xmr`_CBJ^$;xnZ2chg{6xz z*wn$s%F@-1-OUkf|1Z%*uO-Xh^$$_S1TOR*PMq+oyO>1bJOYMpNgz8`_$yTE9eZrw zbOYLG!pBa<*zpa4maFgnCl2p@F^*9rXunp{r1_RMS(yXraD`a8cIA;an55-GK~w@6 zLgpGFHx-3&WXz_e5(x|bnu>;o9)mZ~V$gADQzB={M<>y&qV7ZrY;?mH-&T^fL%K(6 z&Txs_ZF)cH6UwQ7>4b3@1Zzk1q%8bE_;<$u02KhsDY@L>DFE9}008H&j{lV#7rWP= zZT^`k;2&)s3)?wv3nZ*zw>*yT|MHt_+F?*3F(DU>MMiFqJ?w0M7r{D2;Mi^tIhK2M zcBss;9{_Ccmt&5pKQ8?`qu#1l8+>e6- zM-hs3ucHl}M$7_ICKEXu<$)|vj0bU*$iw=nF%AnOqPF?c-m!B^_g#M*D9JN-u7ctMU)nVhlEfbvqn|egrX7& zhzrnUNTv3w>dzrY+t594dcU}7j2}sj^E$S`4U=lnDa;Rk69@kJ;rM1*1u8_B3d4Yn zYo{br_^m(!-q!)fB~n*hR56gC9@8SZ{3tlW78+Uwycyi7Zr5M@)xmIUj_JGe3%d0< zqPh8@$>CNkJm9)!|5+DRg62(De9;C-$~3DmZ(swXC^_GXF#tJcT8d;S*oFzK2T)A! zUj$Vk<`T2q50YI>V_k4;DQb$0=kC^~2)zAv#SvJG!)R|W=s$0T%4Zmlsn>`)kT{ih zZxLU~oTSXc{j833sVN{e#D#H;HjbZ-QGzly1t`Opm@rBQC4f=_E`x-%^pL#XO}d~; zEsEKCEoqk;h~eV^)*vn!xbGJw+da^@q8oRi>37o%Vjc8F1XM6@E)w`>YPN#AYMpZK zKGk-qbV)XiizjwJndvT!Fd=lfRHeIK&pbEb%zz=efC;N>s}c$F`nZU;{L?vlCQWh< z4kmeKaU;pwW0$cof)?8%N6B>VpQCr&GOG}%n>Q*+ZN*0HfT|XhHav_6{fr-#zq;GuF>oX;+6;{& zwTz%_C;J?MBC$ed4jL|N(B>fx^R;P(Qg8F+!YVoH(5XA(l6&-q$b>&*!IT2W1M5my zgFh3LJ6;KcH(io^hGQ<25KJRg!Ws^2i6Lf43o!)rcAJ}B3_9IYo4ES7j;aRirKsMg zKGE_+=w5*E{{HF1zsjCj;fWKvR_uU44nYMVke{I#x=hJWh<4K9u2hScBR< z%uYZ6np5N?zL*+Qh`pkzv+)X!7F)7+PUJWmNS37_s&B=_SKGY;?BM<`ZIyIWYdb4y0&^uRhN+=b6rl(393&T4L>0RM%{z=5wmT2qo}#i zi?E@JGs{GI(-B#vlq4|-+_V=pJg)x06{|5m{_HxHCG}$V#(GgALzJjzc|;R9D=IH4 z=Fn2+O+GDbiuU@2Fq5n@U3cUHIj{_gIdB{;Lda4D@jZo3m)*Q)R$bm9DCSyWU26WU z?h9hI%hx=Gf-owyZy0jZjRi+&Bm?c4qb@PtLHF8eYlR?MW)dmWCJ7@G(S`tN)!fQ5 z5L~kwoLeauG15zD-H-mYx$8-w*30yrYXN_Hd^VfWZp*b7Q}(>T#!PeLZRu7-3!@_W zcnL>Z-ish2eU-(FU-wCB*xAJVJW{)2l`82rjtqITeYxe#RuZBpy?lGL#N>ms{hf$w zs1srM7$B3e)v%B(Ms=!MK6qfXua^a5lCoWjMoOv00;rf2i$-F9v5dr!g__Sy7(cmX z_xn2v?^2?6u9A8+9tR0hfk9z=lv3--7AED^{v_w;`O(L0w8mDasQhKd@ zZ<396-DD&baffzVkIRLRD>=gsDl`qt^3;_Ren>A<-1Ou;rp{bVd?!}2;B#3;V+x&A z2wD3Gai&l=`D6!Qi@U7%JRCkpnXN@nTSbXi12p)af*uER!4xTSPyMy>_-Ez*5#TVI z6{(l{@vl6Rw(YDa^w0bu3Gl}PSclzk7YRgk_ySmQWf{&{V!+<_EbGIoc)cwQMArom zUssHjin{>{t`30ND?XPQog2(IAEx;^_aN|jCCQA;voxVO2V{%rg{HTa>2N=sv}w`;Br>x%TGHvb3Q zh0AmwYeyzlsSV8+)eKiz%!%#%Q%KWnx{hrJ-8lOFk$$hzb~G+i(Y>yLO1~zNNL$f( zFva{3zO%+K_FbQLC&seJbTFmG7dmc3+L*;uv?t(q%dB!W(ACy0A!UQAn$Z9@CI{z5os7ivocWR6EtIl@eT(){iibm?e;d zw6?28uf!k<>qyUx2uVpH%E*x_`<;-V-I=t609QPQkcjSd#a6FB^JF4SJffWPxY4y$ zWG6!A5n%vnV9f7+*P&D2)9#Fi-coSGO5tFb|L3gRr6Z3k8LO+v6&nZ zQ%GORejKNwwGMJoX4R#k>EicKt1JC^*#<=m8!lct?REKSHTz3T!RbbGj`-*GL5yO} zZeW+o{E_y2j&V(MT7Lv-ItF5qW`Ba6zPIypY+1rfu&VjdQ5X#SmbsRoo%17MB*_9F zpIfAB;O%{JstJ1#C2&S23I&gWnogV8BAh?&eXX(ch=nymu{={9E6_hzl9dz~{|vb& z+Z6-&dV8o#Th#I43Hp zUYO4FK%|PK==!d&4GyOYV+sr=bX#6XRk%M#5b0HR9@4t1{oLf&^@BhZ?QP;_mfaZ} zkIElYT=+e;XcQMPXj4g0F3Xka?Q4fU+tM6rhue+hn{_O`!Ivmjo;@yP9wXu>ZcG7% z>l_&Nt@yJu=6fIA^vyZUCLK-uX=A3$8CcwlQN8DB${1>Mo(Fh&=x`(_d9QinGys!d zvQ)f6CI2bTfdS^F8N!&xT${4l)sd`Rvf^N2?Z>gb5f%eG%552x+A_;&;I0RKqXoie z$=FXpoWyC_g0!$rHmwm6BPp!Ek~hPuFg1;3{V$yczp_<&L7d)RwQ={Sk* zoQN2`Tk^W=!`@Dmp?<8TKQr#8MEPjY$gOp8)5#}n3h)l^_-+AMWO28k6P^O>QDa2` z?x$JOL}_7g;5g<%sDuW9UY*AY_A8fM2|E_+R7Zs)_ickH@l30t;&bw-33ceIGZ%H) ztO;thsOsB2RF}lG4ma7Zrdh*Zv)s-Y8ldehloY{%% z9wk*+82p*mk53Z;B90%}B|aMr96sD1l=2nBf0N^5N^UA1j|!jZL*ckRI#zLF6ca1G zTd8CAZUv%$WLWPRB8Sk>MDW<~DoR_w-^thxTU59}CU`W`U8r1UBST)rcvgDla{C2^ z`b7!})+}`rKVjwa+n2_lTdGzfYf0u#7F(1sGtxpPo5@d0cQDi+WT~v8ddS9h zy%^t6Cei9@l(MoF!Wy$4;jk`zx_*UPQim7AeCFbQJkRda6xPDzCxDx{tfU)o z@?_M5P^-){ zmI#C1q$I-Wm`T6WN$>uYENKR^p?Z4w5h4nzZ^i`k{mPb~Bci7BTZ2mrv8%gv9CLo? zznG0}%#LpaXL>5eYA4AB79=*hIG$mk6uTTWQ76;{JgBcuS!AQ_-l|Sn8Zt#Re5BVH z>3d!Zbd{7Du6PqYM3_wXeZgOkJzf;t>-x)oEYUj$$qXTs?jtQe6?87(Os@nD7$Y$I zf&j_Q#M2=TFR8O}P+c6a;XD{(>&`sQK=IZq4FCnpn~_qo-i0&DbNi#}H5qrmAib7* z7BJnPJ!dcNKF-g^W$`?E6AyYb>ZmzIwwF@_Zh6yAuqD83l9qF-X*67>h5RbUuf+0x z{+nJQgNDR~JrTjrS_n!=pK}?<&$;CWc_$n~)!doW0@wi|6?CAVmhAa9EMuK3re24| zFx2ySk`KLu;W#+%eudfsB*C`zaYG3y9E}W4LC6u z4oLfwqW*o&_~qoL1}ooI8cJPq{rI`e13QHs3&mHd2}`N!>vZh^5A;ztyS#V74=X#8 z_l82^xQ$+Ty0!1mg`$AbQ0b{;^=3YreO&ex+t<2h4mO7j` zVh_Q!4Q_Ld;~P1&d|tRA<^db-qdi?F z7G%-)IZ?hY5=D~?jvWuHliUBfdf$I^mj+%sy7O&5gAnn+ZlUw%hWv9MmzK(3Sd?!X zXg&}KFF9x}q&ya;uFYCBaoGw>fO#5*iOTyqAE@#Q!CWc|YCO14>5 z&}i#;nfxwXe75Sck&bC3mPl-IrW3R!HyzTB3+4&o>)e~ymY6_KE)OPKQ&Yj6`aBeS zLV##qpSbcCLIkz<-Q-RJVT?G7AXgJH@a((b2IPf^<5z7eZWB*#jYU^#MK9I0%A{WV z6{fIN-kyvsVzrXQiA-sBI_}f2Rz?N50$&uHh|M=KALsclYVB;UvyQ#r!8{&lD#$b^ zY-?KiI@}wZP5UZ!!e6fw+0mURcCGRb3xcxxZ5gwXAQ-4=Q7 zvKV+Y1xXdOx>ph^$?jN(=K>9U_2k7#Av_-vl}jd6+Dz*q(-%Zi)r_ zz*b$9>FFem5+A1+`+#}+K*kiv3JEwa&+?We|BEZP5-Kq0o{8uZi__!MXaIz4wBEpM zN}B(9w51XI5q4*HA=#dXiTTH0IX$EPxB8( zHu&W7pU$bGvLuPVN>UWaPbve$lF$>B!(!9BMkbVJuJSWYbe*yA{`E}bL^FY>bhQBYt!k(Cv$1@ilk?ZH2c5|etYue97iR+( zFtmWxmb9aUx$5m-UH|0U>y=GBgf5@7 zW=mP^*1M${=`)8EBrng4)7SPc-_ea};{xv#ifypIe7O^1_mK8%CRkO+veikv0eh}a zs4v_yTrPFr9UeB9q2Oh;nXQoBpFP6;n^OMfy2K`FWaqLjZ0@9+8t zrL31_E@{}>QB$cw8}``;GR?(f<^z*`Y(X_$>9Hi%YQfcPlEez186Y!lg}TQ v{X5TnQhhT1C(Hf2>VGEwchz}<|D*b6{{JluB;?;_h)+ZJ={AytzjyxwWtHh* literal 0 HcmV?d00001 diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index 4813536f..45b36830 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -157,6 +157,20 @@ class SubmitFileTest(TestCase): {'source': ['Only .zip files are accepted.']}, ) + def test_validation_errors_more_than_one_theme_xml(self): + self.assertEqual(Extension.objects.count(), 0) + user = UserFactory() + self.client.force_login(user) + + with open(TEST_FILES_DIR / 'theme-more-than-one-xml.zip', 'rb') as fp: + response = self.client.post(self.url, {'source': fp, 'agreed_with_terms': True}) + + self.assertEqual(response.status_code, 200) + self.assertDictEqual( + response.context['form'].errors, + {'source': ['A theme ZIP archive should contain a single XML file']}, + ) + def test_theme_file(self): self.assertEqual(File.objects.count(), 0) user = UserFactory() diff --git a/files/forms.py b/files/forms.py index 7de275b5..13a03afb 100644 --- a/files/forms.py +++ b/files/forms.py @@ -4,6 +4,7 @@ import zipfile import tempfile from django import forms +from django.core.exceptions import ValidationError from django.utils.safestring import mark_safe from django.utils.translation import gettext_lazy as _ @@ -12,10 +13,7 @@ from .validators import ( FileMIMETypeValidator, ManifestValidator, ) -from constants.base import ( - EXTENSION_TYPE_SLUGS_SINGULAR, - ALLOWED_EXTENSION_MIMETYPES, -) +from constants.base import EXTENSION_TYPES, ALLOWED_EXTENSION_MIMETYPES import files.models import files.utils as utils @@ -28,6 +26,14 @@ logger = logging.getLogger(__name__) class FileForm(forms.ModelForm): msg_only_zip_files = _('Only .zip files are accepted.') + # Mimicking how django.forms.fields.Field handles validation error messages. + # TODO: maybe this should be a custom SourceFileField with all these validators and messages + error_messages = { + 'invalid_manifest_toml': _('A valid manifest file could not be found'), + 'invalid_zip_archive': msg_only_zip_files, + 'invalid_theme_multiple_xmls': _('A theme ZIP archive should contain a single XML file'), + } + class Meta: model = files.models.File fields = ('source', 'type', 'metadata', 'agreed_with_terms', 'user') @@ -38,7 +44,7 @@ class FileForm(forms.ModelForm): validators=[ FileMIMETypeValidator( allowed_mimetypes=ALLOWED_EXTENSION_MIMETYPES, - message=msg_only_zip_files, + message=error_messages['invalid_zip_archive'], ), ], widget=forms.ClearableFileInput( @@ -128,22 +134,23 @@ class FileForm(forms.ModelForm): errors = [] if not zipfile.is_zipfile(file_path): - errors.append('File is not .zip') + errors.append(ValidationError(self.error_messages['invalid_zip_archive'])) - manifest = utils.read_manifest_from_zip(file_path) + manifest, error_codes = utils.read_manifest_from_zip(file_path) + for code in error_codes: + errors.append(ValidationError(self.error_messages[code])) if manifest is None: - errors.append('A valid manifest file could not be found') + errors.append(ValidationError(self.error_messages['invalid_manifest_toml'])) else: ManifestValidator(manifest) ExtensionIDManifestValidator(manifest, self.extension) - extension_types = {v: k for k, v in EXTENSION_TYPE_SLUGS_SINGULAR.items()} if errors: - raise forms.ValidationError({'source': errors}, code='invalid') + self.add_error('source', errors) self.cleaned_data['metadata'] = manifest # TODO: Error handling - self.cleaned_data['type'] = extension_types[manifest['type']] + self.cleaned_data['type'] = EXTENSION_TYPES[manifest['type']] return self.cleaned_data diff --git a/files/utils.py b/files/utils.py index c02008e8..3c09d188 100644 --- a/files/utils.py +++ b/files/utils.py @@ -1,3 +1,5 @@ +import typing + from pathlib import Path import hashlib import io @@ -59,30 +61,51 @@ def find_file_inside_zip_list(file_to_read: str, name_list: list) -> str: return None +def find_files_inside_zip_list(file_extension: str, name_list: list) -> typing.Iterable[str]: + """Return a generator of list of files with a given extension a ZIP name_list.""" + for file_path in name_list: + # Remove leading/trailing whitespace from file path + file_path_stripped = file_path.strip() + # Get file's extension + _, file_path_ext = os.path.splitext(file_path_stripped) + # Check if this file's extension matches the extension we are looking for + if file_path_ext.lower() == file_extension.lower(): + yield file_path_stripped + + def read_manifest_from_zip(archive_path): file_to_read = 'blender_manifest.toml' + error_codes = [] try: with zipfile.ZipFile(archive_path) as myzip: - manifest_filepath = find_file_inside_zip_list(file_to_read, myzip.namelist()) + file_list = myzip.namelist() + manifest_filepath = find_file_inside_zip_list(file_to_read, file_list) if manifest_filepath is None: logger.info(f"File '{file_to_read}' not found in the archive.") - return None + return None, error_codes # Extract the file content with myzip.open(manifest_filepath) as file_content: - # TODO: handle TOML loading error toml_content = toml.loads(file_content.read().decode()) - return toml_content + + # In case manifest TOML was successfully parsed, do additional type-specific validation + if toml_content['type'] == 'theme': + theme_xmls = find_files_inside_zip_list('.xml', file_list) + if len(list(theme_xmls)) != 1: + error_codes.append('invalid_theme_multiple_xmls') + + return toml_content, error_codes except toml.decoder.TomlDecodeError as e: - # TODO: This error should be propagate to the user logger.error(f"Manifest Error: {e.msg}") + error_codes.append('invalid_manifest_toml') except Exception as e: logger.error(f"Error extracting from archive: {e}") + error_codes.append('invalid_zip_archive') - return None + return None, error_codes def guess_mimetype_from_ext(file_name: str) -> str: -- 2.30.2 From 829145c2e1b8a0a5735bd470503e156614c5d146 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Mon, 8 Apr 2024 19:17:09 +0200 Subject: [PATCH 02/11] Fix a typo, rephrase the util func --- files/utils.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/files/utils.py b/files/utils.py index 3c09d188..c6d4a78d 100644 --- a/files/utils.py +++ b/files/utils.py @@ -51,7 +51,7 @@ def get_sha256_from_value(value: str): def find_file_inside_zip_list(file_to_read: str, name_list: list) -> str: - """Return the first occurance of file_to_read insize a zip name_list""" + """Return the first occurrence of file_to_read insize a zip name_list""" for file_path in name_list: # Remove leading/trailing whitespace from file path file_path_stripped = file_path.strip() @@ -61,15 +61,15 @@ def find_file_inside_zip_list(file_to_read: str, name_list: list) -> str: return None -def find_files_inside_zip_list(file_extension: str, name_list: list) -> typing.Iterable[str]: - """Return a generator of list of files with a given extension a ZIP name_list.""" - for file_path in name_list: +def filter_file_paths_by_ext(paths: typing.Iterable[str], extension: str) -> typing.Iterable[str]: + """Generate a list of paths having a given extension from a given list of file paths.""" + for file_path in paths: # Remove leading/trailing whitespace from file path file_path_stripped = file_path.strip() - # Get file's extension + # Get file path's extension _, file_path_ext = os.path.splitext(file_path_stripped) # Check if this file's extension matches the extension we are looking for - if file_path_ext.lower() == file_extension.lower(): + if file_path_ext.lower() == extension.lower(): yield file_path_stripped @@ -91,7 +91,7 @@ def read_manifest_from_zip(archive_path): # In case manifest TOML was successfully parsed, do additional type-specific validation if toml_content['type'] == 'theme': - theme_xmls = find_files_inside_zip_list('.xml', file_list) + theme_xmls = filter_file_paths_by_ext(file_list, '.xml') if len(list(theme_xmls)) != 1: error_codes.append('invalid_theme_multiple_xmls') -- 2.30.2 From a64eb5b9ad23f1dfda9e98f0dd3e63d992e8809a Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Tue, 9 Apr 2024 10:24:47 +0200 Subject: [PATCH 03/11] Check for an __init__.py if a dir is found --- constants/base.py | 2 +- .../tests/files/addon-single-py-module.zip | Bin 0 -> 558 bytes extensions/tests/files/addon-without-init.zip | Bin 0 -> 752 bytes extensions/tests/test_submit.py | 41 +++++++++++++++++- files/forms.py | 7 +-- files/utils.py | 9 +++- 6 files changed, 53 insertions(+), 6 deletions(-) create mode 100644 extensions/tests/files/addon-single-py-module.zip create mode 100644 extensions/tests/files/addon-without-init.zip diff --git a/constants/base.py b/constants/base.py index 251a3120..e9c80385 100644 --- a/constants/base.py +++ b/constants/base.py @@ -58,7 +58,7 @@ EXTENSION_TYPE_PLURAL = { EXTENSION_TYPE_CHOICES.THEME: _('Themes'), } EXTENSION_SLUGS_PATH = '|'.join(EXTENSION_TYPE_SLUGS.values()) -EXTENSION_TYPES = {v: k for k, v in EXTENSION_TYPE_SLUGS_SINGULAR.items()} +EXTENSION_SLUG_TYPES = {v: k for k, v in EXTENSION_TYPE_SLUGS_SINGULAR.items()} ALLOWED_EXTENSION_MIMETYPES = ('application/zip', ) # FIXME: this controls the initial widget rendered server-side, and server-side validation diff --git a/extensions/tests/files/addon-single-py-module.zip b/extensions/tests/files/addon-single-py-module.zip new file mode 100644 index 0000000000000000000000000000000000000000..740e82fc438da788a42cdd2ff995647d1e6a8847 GIT binary patch literal 558 zcmWIWW@h1H0D*}Coe`zW|6URWvO$=aL586?KQ}c#F(oBGPp_adG=!6ZdEz&bG!QPW z;AUWCdCAPc!14-66clCVm1wAEq~_%0E0pIK<)o-d*76F>X!Jrt}8KL<2^`8Ae zo&qDtd>MwMoYcIO)S~#@#JtS3)Z!AolKk8pupRThi==S_F`6B|eTLkJ3`APK|LK~r zci}X@re>vG79zVilvK_7%`WV@q9Xe1z3(M4*?UFb_wBCcZ4k=&%=miBK{xFs?5Diq zKFkr+-{q+2veZm!#RMZ`k7l36=Q<``EIydsls;ji#Ex zq2^ToMXgTqd^t<}PqRe*_S8De!Sy%WW|^B?% Date: Tue, 9 Apr 2024 18:55:29 +0200 Subject: [PATCH 04/11] Clean up --- extensions/tests/files/addon-without-dir.zip | Bin 0 -> 708 bytes ...init.zip => invalid-addon-dir-no-init.zip} | Bin ...y-module.zip => invalid-addon-no-init.zip} | Bin .../files/{not_a.zip => invalid-archive.zip} | 0 .../tests/files/invalid-manifest-path.zip | Bin 0 -> 679 bytes .../tests/files/invalid-no-manifest.zip | Bin 0 -> 197 bytes ...ml.zip => invalid-theme-multiple-xmls.zip} | Bin extensions/tests/test_manifest.py | 12 ++ extensions/tests/test_submit.py | 111 +++++------------- files/forms.py | 20 ++-- files/tests/test_utils.py | 61 +++++++++- files/utils.py | 65 +++++++--- 12 files changed, 157 insertions(+), 112 deletions(-) create mode 100644 extensions/tests/files/addon-without-dir.zip rename extensions/tests/files/{addon-without-init.zip => invalid-addon-dir-no-init.zip} (100%) rename extensions/tests/files/{addon-single-py-module.zip => invalid-addon-no-init.zip} (100%) rename extensions/tests/files/{not_a.zip => invalid-archive.zip} (100%) create mode 100644 extensions/tests/files/invalid-manifest-path.zip create mode 100644 extensions/tests/files/invalid-no-manifest.zip rename extensions/tests/files/{theme-more-than-one-xml.zip => invalid-theme-multiple-xmls.zip} (100%) diff --git a/extensions/tests/files/addon-without-dir.zip b/extensions/tests/files/addon-without-dir.zip new file mode 100644 index 0000000000000000000000000000000000000000..0c488cefe2827d3e4ffbade2b6b1c2f8dd01f125 GIT binary patch literal 708 zcmWIWW@h1H0D%WnJ0rjhD8bDj!w?^znU`4-AFo$X85+XLz|2x8ng+t972FJrEH9ZE z7+78bi2%4E69YOUN|*n=BnmVHgn5C66zAur#wVtvMe(_bd6{Xc z#U*+r`MEh@r_TE>k_N(PcKG%gavw4fY5D%AYr@`z)BKv6m3CQ(?BY;THS0IKu;+@3 z=&$#_m&9c66@A~gyPCH_DCaZd>nR7_w3o1-@{0Q~M^Jy4qoT`FGpQ95jEp^+eHNeV zm~^rDV0Kgbgo%z@8}yX;!@{5c{ugz3lfX2|*}R9EQ~ejUI?40pEb%|h67}0t>o5n` z-)x&@Zf-HRo5gOQ$#`F}&#%N=a*z4}GtnQFK7vV0|8JP{Ug_Gb!fmTw*CuuE(RjJ- zagy4UL+$@3d|?gnW@M6M#uZ@_z>tA}C5<2&CHhz)(T5fu$Od9Y9>hRkSTihXbOn-V vQ3*5>OEf}^#1&@24*1q52V+FoW=kE6V%V6 literal 0 HcmV?d00001 diff --git a/extensions/tests/files/addon-without-init.zip b/extensions/tests/files/invalid-addon-dir-no-init.zip similarity index 100% rename from extensions/tests/files/addon-without-init.zip rename to extensions/tests/files/invalid-addon-dir-no-init.zip diff --git a/extensions/tests/files/addon-single-py-module.zip b/extensions/tests/files/invalid-addon-no-init.zip similarity index 100% rename from extensions/tests/files/addon-single-py-module.zip rename to extensions/tests/files/invalid-addon-no-init.zip diff --git a/extensions/tests/files/not_a.zip b/extensions/tests/files/invalid-archive.zip similarity index 100% rename from extensions/tests/files/not_a.zip rename to extensions/tests/files/invalid-archive.zip diff --git a/extensions/tests/files/invalid-manifest-path.zip b/extensions/tests/files/invalid-manifest-path.zip new file mode 100644 index 0000000000000000000000000000000000000000..afc4e4981b94f77211befb6fb91044c1b78cd1ed GIT binary patch literal 679 zcmWIWW@h1H0D+lhoe^LLlwe_yVaU%*)ejBfWMEdZ5>1l_;?fFk21b^b%nS@HuYg1V zT=!fw-5gNeCFS`L6EuOkL8f3aK?G=71eG@4HAE2%{P6+h@pq$Uvm!`=71}dlydgYid^7Wg)VQLrK-F z-|WJkD=MPD-uqq>lf75;ec$eC-UgwZ&y2699CXuO!hXst?!z2G{aucVE=$d%R!lH5 z_GtE5e6C~C#o~k6P3aRRI&N*yQ{oQ`fBySl)ZI-2(p>Ick3e^mMiCN2HHVa|J{YqJWst$JOX z)V)XJ<+jI3YEuri|DW)MHNcyZNsbv;yhuP}NPyw3BZvu!C00l*p#=o8NtkhjY?2++ zBuI<_O#{UkR?~3B7sNDR)G=&nY(O#%N9+Nuz!Gx--mGjuy$sAi_!~%11DVGF0QaJ~ A;Q#;t literal 0 HcmV?d00001 diff --git a/extensions/tests/files/invalid-no-manifest.zip b/extensions/tests/files/invalid-no-manifest.zip new file mode 100644 index 0000000000000000000000000000000000000000..d946f997b3a4f57076509fa4d130370bd62124c5 GIT binary patch literal 197 zcmWIWW@h1H00Fkj&WO_Ge=msw*&xizAj43cpPL$=n39s8r&mxJ8p6rIyxUeZ4TMW8 zxEUB(UNSQ str: - """Return the first occurrence of file_to_read insize a zip name_list""" - for file_path in name_list: +def find_path_by_name(paths: typing.List[str], name: str) -> typing.Optional[str]: + """Return the first occurrence of file name in a given list of paths.""" + for file_path in paths: # Remove leading/trailing whitespace from file path file_path_stripped = file_path.strip() # Check if the basename of the stripped path is equal to the target file name - if os.path.basename(file_path_stripped) == file_to_read: + if os.path.basename(file_path_stripped) == name: return file_path_stripped return None -def filter_file_paths_by_ext(paths: typing.Iterable[str], extension: str) -> typing.Iterable[str]: - """Generate a list of paths having a given extension from a given list of file paths.""" +def find_full_path( + paths: typing.List[str], *full_path_components: typing.List[str] +) -> typing.Optional[str]: + """Return a path equal given path components if it exists in a given list of paths.""" + matching_paths = (path for path in paths if path == os.path.join(*full_path_components)) + return next(matching_paths, None) + + +def filter_paths_by_ext(paths: typing.List[str], ext: str) -> typing.Iterable[str]: + """Generate a list of paths having a given extension from a given list of paths.""" for file_path in paths: # Remove leading/trailing whitespace from file path file_path_stripped = file_path.strip() # Get file path's extension _, file_path_ext = os.path.splitext(file_path_stripped) # Check if this file's extension matches the extension we are looking for - if file_path_ext.lower() == extension.lower(): + if file_path_ext.lower() == ext.lower(): yield file_path_stripped def read_manifest_from_zip(archive_path): - file_to_read = 'blender_manifest.toml' + """Read and validate extension's manifest file and contents of the archive. + + In any extension archive, a valid `blender_manifest.toml` file is expected + to be found at the top level of the archive, or inside a single nested directory. + Additionally, depending on the extension type defined in the manifest, + the archive is expected to have a particular file structure: + + * for themes, a single XML file is expected next to the manifest; + + * for add-ons, the following structure is expected: + + ``` + some-addon.zip + └─ an-optional-dir + ├─ blender_manifest.toml + ├─ __init__.py + └─ (...) + ``` + """ + manifest_name = 'blender_manifest.toml' error_codes = [] try: with zipfile.ZipFile(archive_path) as myzip: file_list = myzip.namelist() - manifest_filepath = find_file_inside_zip_list(file_to_read, file_list) + manifest_filepath = find_path_by_name(file_list, manifest_name) if manifest_filepath is None: - logger.info(f"File '{file_to_read}' not found in the archive.") + logger.info(f"File '{manifest_name}' not found in the archive.") return None, error_codes + # Manifest file is expected to be no deeper than one directory down + if os.path.dirname(os.path.dirname(manifest_filepath)) != '': + error_codes.append('invalid_manifest_path') + # Extract the file content with myzip.open(manifest_filepath) as file_content: toml_content = toml.loads(file_content.read().decode()) - is_a_directory = '/' in manifest_filepath - # In case manifest TOML was successfully parsed, do additional type-specific validation + # If manifest was parsed successfully, do additional type-specific validation type_slug = toml_content['type'] if type_slug == 'theme': - theme_xmls = filter_file_paths_by_ext(file_list, '.xml') + theme_xmls = filter_paths_by_ext(file_list, '.xml') if len(list(theme_xmls)) != 1: error_codes.append('invalid_theme_multiple_xmls') elif type_slug == 'add-on': - if is_a_directory: - init_filepath = find_file_inside_zip_list('__init__.py', file_list) - if not init_filepath: - error_codes.append('invalid_missing_init') + # __init__.py is expected to be next to the manifest + init_directory = os.path.dirname(manifest_filepath) + init_filepath = find_full_path(file_list, init_directory, '__init__.py') + if not init_filepath: + error_codes.append('invalid_missing_init') return toml_content, error_codes -- 2.30.2 From aef236cf25a04c8d8d91c4d28aab810da757a95b Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Tue, 9 Apr 2024 19:06:50 +0200 Subject: [PATCH 05/11] Test for error message on invalid TOML --- extensions/tests/files/invalid-manifest-toml.zip | Bin 0 -> 934 bytes extensions/tests/test_submit.py | 1 + files/forms.py | 8 +++----- files/utils.py | 1 + 4 files changed, 5 insertions(+), 5 deletions(-) create mode 100644 extensions/tests/files/invalid-manifest-toml.zip diff --git a/extensions/tests/files/invalid-manifest-toml.zip b/extensions/tests/files/invalid-manifest-toml.zip new file mode 100644 index 0000000000000000000000000000000000000000..942089d40e54a3660db0f9634251a7cdc800fd3d GIT binary patch literal 934 zcmWIWW@h1H00H9}oe^LLl;CENVJObeP1Q|INy*RC4-MgDU@j~XO$!F%(h6<{MwXY% z3=Ax*y2-wMhTMk?1X{oU=~Axt*7a*@R@!C35z8`BX!c>Y*}G>gar(d4bM=b}Pxs#W zzAw6c$;GSsCHD-jUK3d3sQ1}Oe+kE{dy3Z+BR& zY4F|?-TjN^AOB#fV4BI0*C+2WS$WHgfR>H(XS{0u{aECuQlZp4_w;wUGZIn*^&V<( zd)~N(>tlUMJu@hXAmRLADrz{3p$1WWd}dx|NqoFsK_xg~SPDhcKp0DqL(H2P&>2y> z{O=`EV332b1gd$UD2xY3Ay-$c?t7|q0jqRhM!4fTxFoSb}x^8BKl6m?Cm0B=Sn zIc8i5L;`3X2naB|bp+8U$%Pe?T+nQR7>FyeKnw(?8-^{7uBZm$NILjz!4=*xXEQJ? zY0LmxhU9FN& Date: Tue, 9 Apr 2024 19:23:01 +0200 Subject: [PATCH 06/11] Shouldn't be any trailing spaces in archive listing, and that util should fail if there are any instead --- files/utils.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/files/utils.py b/files/utils.py index cefcdf43..ea9a7bce 100644 --- a/files/utils.py +++ b/files/utils.py @@ -72,13 +72,11 @@ def find_full_path( def filter_paths_by_ext(paths: typing.List[str], ext: str) -> typing.Iterable[str]: """Generate a list of paths having a given extension from a given list of paths.""" for file_path in paths: - # Remove leading/trailing whitespace from file path - file_path_stripped = file_path.strip() # Get file path's extension - _, file_path_ext = os.path.splitext(file_path_stripped) + _, file_path_ext = os.path.splitext(file_path) # Check if this file's extension matches the extension we are looking for if file_path_ext.lower() == ext.lower(): - yield file_path_stripped + yield file_path def read_manifest_from_zip(archive_path): -- 2.30.2 From 7db8fe1680d255aac3d31f63f7976088d533cfef Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 11 Apr 2024 11:25:57 +0200 Subject: [PATCH 07/11] Clean up --- files/forms.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/files/forms.py b/files/forms.py index 20599b66..68497e60 100644 --- a/files/forms.py +++ b/files/forms.py @@ -4,7 +4,6 @@ import zipfile import tempfile from django import forms -from django.core.exceptions import ValidationError from django.utils.safestring import mark_safe from django.utils.translation import gettext_lazy as _ @@ -140,7 +139,7 @@ class FileForm(forms.ModelForm): errors = [] if not zipfile.is_zipfile(file_path): - errors.append(ValidationError(self.error_messages['invalid_zip_archive'])) + errors.append(forms.ValidationError(self.error_messages['invalid_zip_archive'])) manifest, error_codes = utils.read_manifest_from_zip(file_path) @@ -152,7 +151,7 @@ class FileForm(forms.ModelForm): self.cleaned_data['type'] = EXTENSION_SLUG_TYPES[manifest['type']] for code in error_codes: - errors.append(ValidationError(self.error_messages[code])) + errors.append(forms.ValidationError(self.error_messages[code])) if errors: self.add_error('source', errors) -- 2.30.2 From 34dfb0bb475f3335e80584ca73a329a636462b39 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 11 Apr 2024 11:52:03 +0200 Subject: [PATCH 08/11] Stop validation of manifest path looks invalid --- extensions/tests/test_manifest.py | 4 ++-- extensions/tests/test_submit.py | 1 - files/forms.py | 11 +++++------ files/utils.py | 1 + 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/extensions/tests/test_manifest.py b/extensions/tests/test_manifest.py index 701aacbb..9b50158d 100644 --- a/extensions/tests/test_manifest.py +++ b/extensions/tests/test_manifest.py @@ -271,7 +271,7 @@ class ValidateManifestTest(CreateFileTest): self.client.force_login(user) file_data = { - "id": "id-with-hyphens", + "id": "id-with-hyphens", } bad_file = self._create_file_from_data("theme.zip", file_data, self.user) @@ -283,7 +283,7 @@ class ValidateManifestTest(CreateFileTest): self.assertEqual(response.status_code, 200) error = response.context['form'].errors.get('source') self.assertEqual(len(error), 1) - self.assertIn('"<b>id-with-hyphens</b>"', error[0]) + self.assertIn('"id-with-hyphens"', error[0]) class ValidateManifestFields(TestCase): diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index be0a4f86..8d5a654d 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -66,7 +66,6 @@ EXPECTED_VALIDATION_ERRORS = { 'invalid-manifest-path.zip': { 'source': [ 'The manifest file should be at the top level of the archive, or one level deep.', - 'An add-on should have an __init__.py file.', ], }, 'invalid-addon-no-init.zip': { diff --git a/files/forms.py b/files/forms.py index 68497e60..b0890314 100644 --- a/files/forms.py +++ b/files/forms.py @@ -139,9 +139,13 @@ class FileForm(forms.ModelForm): errors = [] if not zipfile.is_zipfile(file_path): - errors.append(forms.ValidationError(self.error_messages['invalid_zip_archive'])) + raise forms.ValidationError(self.error_messages['invalid_zip_archive']) manifest, error_codes = utils.read_manifest_from_zip(file_path) + for code in error_codes: + errors.append(forms.ValidationError(self.error_messages[code])) + if errors: + self.add_error('source', errors) if manifest: ManifestValidator(manifest) @@ -150,9 +154,4 @@ class FileForm(forms.ModelForm): self.cleaned_data['metadata'] = manifest self.cleaned_data['type'] = EXTENSION_SLUG_TYPES[manifest['type']] - for code in error_codes: - errors.append(forms.ValidationError(self.error_messages[code])) - if errors: - self.add_error('source', errors) - return self.cleaned_data diff --git a/files/utils.py b/files/utils.py index ea9a7bce..d9ed7a18 100644 --- a/files/utils.py +++ b/files/utils.py @@ -114,6 +114,7 @@ def read_manifest_from_zip(archive_path): # Manifest file is expected to be no deeper than one directory down if os.path.dirname(os.path.dirname(manifest_filepath)) != '': error_codes.append('invalid_manifest_path') + return None, error_codes # Extract the file content with myzip.open(manifest_filepath) as file_content: -- 2.30.2 From 712388bfb2c1ecb6a39985084faa1030eb4e1ae8 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 11 Apr 2024 12:07:31 +0200 Subject: [PATCH 09/11] Another clean up --- files/tests/test_utils.py | 10 +++++----- files/utils.py | 12 +++++------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/files/tests/test_utils.py b/files/tests/test_utils.py index 83f13973..1a581841 100644 --- a/files/tests/test_utils.py +++ b/files/tests/test_utils.py @@ -1,6 +1,6 @@ from django.test import TestCase -from files.utils import find_path_by_name, find_full_path, filter_paths_by_ext +from files.utils import find_path_by_name, find_exact_path, filter_paths_by_ext class UtilsTest(TestCase): @@ -50,7 +50,7 @@ class UtilsTest(TestCase): manifest_file = find_path_by_name(name_list, self.manifest) self.assertEqual(manifest_file, None) - def test_find_full_path_found(self): + def test_find_exact_path_found(self): name_list = [ 'foobar-1.0.3/theme.xml', 'foobar-1.0.3/theme1.xml', @@ -60,10 +60,10 @@ class UtilsTest(TestCase): 'foobar-1.0.3/foobar-1.0.3/__init__.py', 'blender_manifest.toml', ] - path = find_full_path(name_list, 'foobar-1.0.3', '__init__.py') + path = find_exact_path(name_list, 'foobar-1.0.3/__init__.py') self.assertEqual(path, 'foobar-1.0.3/__init__.py') - def test_find_full_path_nothing_found(self): + def test_find_exact_path_nothing_found(self): name_list = [ 'foobar-1.0.3/theme.xml', 'foobar-1.0.3/theme1.xml', @@ -72,7 +72,7 @@ class UtilsTest(TestCase): 'foobar-1.0.3/foobar-1.0.3/__init__.py', 'blender_manifest.toml', ] - path = find_full_path(name_list, 'foobar-1.0.3', '__init__.py') + path = find_exact_path(name_list, 'foobar-1.0.3/__init__.py') self.assertIsNone(path) def test_filter_paths_by_ext_found(self): diff --git a/files/utils.py b/files/utils.py index d9ed7a18..d8a40b12 100644 --- a/files/utils.py +++ b/files/utils.py @@ -61,11 +61,9 @@ def find_path_by_name(paths: typing.List[str], name: str) -> typing.Optional[str return None -def find_full_path( - paths: typing.List[str], *full_path_components: typing.List[str] -) -> typing.Optional[str]: - """Return a path equal given path components if it exists in a given list of paths.""" - matching_paths = (path for path in paths if path == os.path.join(*full_path_components)) +def find_exact_path(paths: typing.List[str], exact_path: str) -> typing.Optional[str]: + """Return a first path equal to a given one if it exists in a given list of paths.""" + matching_paths = (path for path in paths if path == exact_path) return next(matching_paths, None) @@ -128,8 +126,8 @@ def read_manifest_from_zip(archive_path): error_codes.append('invalid_theme_multiple_xmls') elif type_slug == 'add-on': # __init__.py is expected to be next to the manifest - init_directory = os.path.dirname(manifest_filepath) - init_filepath = find_full_path(file_list, init_directory, '__init__.py') + expected_init_path = os.path.join(os.path.dirname(manifest_filepath), '__init__.py') + init_filepath = find_exact_path(file_list, expected_init_path) if not init_filepath: error_codes.append('invalid_missing_init') -- 2.30.2 From faf55f19251233ae61b17dc801946fbc3e4c5f56 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 11 Apr 2024 12:26:03 +0200 Subject: [PATCH 10/11] Add theme test for multiple XMLs --- extensions/tests/test_submit.py | 1 + files/forms.py | 2 +- files/utils.py | 8 +++++++- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index 8d5a654d..607b8b2e 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -78,6 +78,7 @@ EXPECTED_VALIDATION_ERRORS = { 'source': ['The manifest file is missing.'], }, 'invalid-manifest-toml.zip': {'source': ['Could not parse the manifest file.']}, + 'invalid-theme-multiple-xmls.zip': {'source': ['A theme should have exactly one XML file.']}, } diff --git a/files/forms.py b/files/forms.py index b0890314..31320fdd 100644 --- a/files/forms.py +++ b/files/forms.py @@ -34,7 +34,7 @@ class FileForm(forms.ModelForm): # TODO: surface TOML parsing errors? 'invalid_manifest_toml': _('Could not parse the manifest file.'), 'invalid_missing_init': _('An add-on should have an __init__.py file.'), - 'invalid_theme_multiple_xmls': _('A theme should only have one XML file.'), + 'missing_or_multiple_theme_xml': _('A theme should have exactly one XML file.'), 'invalid_zip_archive': msg_only_zip_files, 'missing_manifest_toml': _('The manifest file is missing.'), } diff --git a/files/utils.py b/files/utils.py index d8a40b12..fbf4debf 100644 --- a/files/utils.py +++ b/files/utils.py @@ -101,6 +101,12 @@ def read_manifest_from_zip(archive_path): error_codes = [] try: with zipfile.ZipFile(archive_path) as myzip: + bad_file = myzip.testzip() + if bad_file is not None: + logger.error('Bad file in ZIP') + error_codes.append('invalid_zip_archive') + return None, error_codes + file_list = myzip.namelist() manifest_filepath = find_path_by_name(file_list, manifest_name) @@ -123,7 +129,7 @@ def read_manifest_from_zip(archive_path): if type_slug == 'theme': theme_xmls = filter_paths_by_ext(file_list, '.xml') if len(list(theme_xmls)) != 1: - error_codes.append('invalid_theme_multiple_xmls') + error_codes.append('missing_or_multiple_theme_xml') elif type_slug == 'add-on': # __init__.py is expected to be next to the manifest expected_init_path = os.path.join(os.path.dirname(manifest_filepath), '__init__.py') -- 2.30.2 From 7cf82cac4a9c5f18785e8a59d49b2aa42b36b6b7 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 11 Apr 2024 12:27:49 +0200 Subject: [PATCH 11/11] Minor change --- files/utils.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/files/utils.py b/files/utils.py index fbf4debf..3c4f413f 100644 --- a/files/utils.py +++ b/files/utils.py @@ -1,5 +1,3 @@ -import typing - from pathlib import Path import hashlib import io @@ -7,6 +5,7 @@ import logging import mimetypes import os import toml +import typing import zipfile from lxml import etree -- 2.30.2