From 9df659b06ecb55bc8fa3a07deb537cd7e4ef3882 Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Fri, 18 Mar 2022 18:01:25 -0500 Subject: [PATCH] fix: better draft name validation. Fixes #3539. (#3671) --- ietf/meeting/tests_views.py | 2 +- ietf/submit/forms.py | 148 +++++++++++++++++++----------------- ietf/submit/tests.py | 46 ++++++----- ietf/submit/utils.py | 1 + ietf/utils/tests.py | 2 +- 5 files changed, 105 insertions(+), 94 deletions(-) diff --git a/ietf/meeting/tests_views.py b/ietf/meeting/tests_views.py index a77884122..0656c2389 100644 --- a/ietf/meeting/tests_views.py +++ b/ietf/meeting/tests_views.py @@ -799,7 +799,7 @@ class MeetingTests(BaseMeetingTestCase): "1. WG status (15 minutes)\n\n2. Status of %s\n\n" % draft2.name) filenames = [] for d in (draft1, draft2): - file,_ = submission_file(name=d.name,format='txt',templatename='test_submission.txt',group=session.group,rev="00") + file,_ = submission_file(name_in_doc=f'{d.name}-00',name_in_post=f'{d.name}-00.txt',templatename='test_submission.txt',group=session.group) filename = os.path.join(d.get_file_path(),file.name) with io.open(filename,'w') as draftbits: draftbits.write(file.getvalue()) diff --git a/ietf/submit/forms.py b/ietf/submit/forms.py index 26f4fb2ec..a96f07644 100644 --- a/ietf/submit/forms.py +++ b/ietf/submit/forms.py @@ -207,83 +207,84 @@ class SubmissionBaseUploadForm(forms.Form): self.add_error('xml', "No docName attribute found in the xml root element") name_error = validate_submission_name(draftname) if name_error: - self.add_error('xml', name_error) - revmatch = re.search("-[0-9][0-9]$", draftname) - if revmatch: - self.revision = draftname[-2:] - self.filename = draftname[:-3] + self.add_error('xml', name_error) # This is a critical and immediate failure - do not proceed with other validation. else: - self.revision = None - self.filename = draftname - self.title = self.xmlroot.findtext('front/title').strip() - if type(self.title) is str: - self.title = unidecode(self.title) - self.title = normalize_text(self.title) - self.abstract = (self.xmlroot.findtext('front/abstract') or '').strip() - if type(self.abstract) is str: - self.abstract = unidecode(self.abstract) - author_info = self.xmlroot.findall('front/author') - for author in author_info: - info = { - "name": author.attrib.get('fullname'), - "email": author.findtext('address/email'), - "affiliation": author.findtext('organization'), - } - elem = author.find('address/postal/country') - if elem != None: - ascii_country = elem.get('ascii', None) - info['country'] = ascii_country if ascii_country else elem.text + revmatch = re.search("-[0-9][0-9]$", draftname) + if revmatch: + self.revision = draftname[-2:] + self.filename = draftname[:-3] + else: + self.revision = None + self.filename = draftname + self.title = self.xmlroot.findtext('front/title').strip() + if type(self.title) is str: + self.title = unidecode(self.title) + self.title = normalize_text(self.title) + self.abstract = (self.xmlroot.findtext('front/abstract') or '').strip() + if type(self.abstract) is str: + self.abstract = unidecode(self.abstract) + author_info = self.xmlroot.findall('front/author') + for author in author_info: + info = { + "name": author.attrib.get('fullname'), + "email": author.findtext('address/email'), + "affiliation": author.findtext('organization'), + } + elem = author.find('address/postal/country') + if elem != None: + ascii_country = elem.get('ascii', None) + info['country'] = ascii_country if ascii_country else elem.text - for item in info: - if info[item]: - info[item] = info[item].strip() - self.authors.append(info) + for item in info: + if info[item]: + info[item] = info[item].strip() + self.authors.append(info) - # --- Prep the xml --- - file_name['xml'] = os.path.join(settings.IDSUBMIT_STAGING_PATH, '%s-%s%s' % (self.filename, self.revision, ext)) - try: - prep = xml2rfc.PrepToolWriter(self.xmltree, quiet=True, liberal=True, keep_pis=[xml2rfc.V3_PI_TARGET]) - prep.options.accept_prepped = True - self.xmltree.tree = prep.prep() - if self.xmltree.tree == None: - self.add_error('xml', "Error from xml2rfc (prep): %s" % prep.errors) - except Exception as e: - msgs = format_messages('prep', e, xml2rfc.log) - self.add_error('xml', msgs) - - # --- Convert to txt --- - if not ('txt' in self.cleaned_data and self.cleaned_data['txt']): - file_name['txt'] = os.path.join(settings.IDSUBMIT_STAGING_PATH, '%s-%s.txt' % (self.filename, self.revision)) + # --- Prep the xml --- + file_name['xml'] = os.path.join(settings.IDSUBMIT_STAGING_PATH, '%s-%s%s' % (self.filename, self.revision, ext)) try: - writer = xml2rfc.TextWriter(self.xmltree, quiet=True) - writer.options.accept_prepped = True - writer.write(file_name['txt']) - log.log("In %s: xml2rfc %s generated %s from %s (version %s)" % - ( os.path.dirname(file_name['xml']), - xml2rfc.__version__, - os.path.basename(file_name['txt']), - os.path.basename(file_name['xml']), - self.xml_version)) + prep = xml2rfc.PrepToolWriter(self.xmltree, quiet=True, liberal=True, keep_pis=[xml2rfc.V3_PI_TARGET]) + prep.options.accept_prepped = True + self.xmltree.tree = prep.prep() + if self.xmltree.tree == None: + self.add_error('xml', "Error from xml2rfc (prep): %s" % prep.errors) except Exception as e: - msgs = format_messages('txt', e, xml2rfc.log) - log.log('\n'.join(msgs)) - self.add_error('xml', msgs) + msgs = format_messages('prep', e, xml2rfc.log) + self.add_error('xml', msgs) - # --- Convert to html --- - try: - file_name['html'] = os.path.join(settings.IDSUBMIT_STAGING_PATH, '%s-%s.html' % (self.filename, self.revision)) - writer = xml2rfc.HtmlWriter(self.xmltree, quiet=True) - writer.write(file_name['html']) - self.file_types.append('.html') - log.log("In %s: xml2rfc %s generated %s from %s (version %s)" % - ( os.path.dirname(file_name['xml']), - xml2rfc.__version__, - os.path.basename(file_name['html']), - os.path.basename(file_name['xml']), - self.xml_version)) - except Exception as e: - msgs = format_messages('html', e, xml2rfc.log) - self.add_error('xml', msgs) + # --- Convert to txt --- + if not ('txt' in self.cleaned_data and self.cleaned_data['txt']): + file_name['txt'] = os.path.join(settings.IDSUBMIT_STAGING_PATH, '%s-%s.txt' % (self.filename, self.revision)) + try: + writer = xml2rfc.TextWriter(self.xmltree, quiet=True) + writer.options.accept_prepped = True + writer.write(file_name['txt']) + log.log("In %s: xml2rfc %s generated %s from %s (version %s)" % + ( os.path.dirname(file_name['xml']), + xml2rfc.__version__, + os.path.basename(file_name['txt']), + os.path.basename(file_name['xml']), + self.xml_version)) + except Exception as e: + msgs = format_messages('txt', e, xml2rfc.log) + log.log('\n'.join(msgs)) + self.add_error('xml', msgs) + + # --- Convert to html --- + try: + file_name['html'] = os.path.join(settings.IDSUBMIT_STAGING_PATH, '%s-%s.html' % (self.filename, self.revision)) + writer = xml2rfc.HtmlWriter(self.xmltree, quiet=True) + writer.write(file_name['html']) + self.file_types.append('.html') + log.log("In %s: xml2rfc %s generated %s from %s (version %s)" % + ( os.path.dirname(file_name['xml']), + xml2rfc.__version__, + os.path.basename(file_name['html']), + os.path.basename(file_name['xml']), + self.xml_version)) + except Exception as e: + msgs = format_messages('html', e, xml2rfc.log) + self.add_error('xml', msgs) except Exception as e: try: @@ -293,6 +294,11 @@ class SubmissionBaseUploadForm(forms.Form): except Exception: self.add_error('xml', "An exception occurred when trying to process the XML file: %s" % e) + # The following errors are likely noise if we have previous field + # errors: + if self.errors: + raise forms.ValidationError('') + if self.cleaned_data.get('txt'): # try to parse it txt_file = self.cleaned_data['txt'] diff --git a/ietf/submit/tests.py b/ietf/submit/tests.py index c892c97b4..b2f03b484 100644 --- a/ietf/submit/tests.py +++ b/ietf/submit/tests.py @@ -83,7 +83,7 @@ class BaseSubmitTestCase(TestCase): def archive_dir(self): return settings.INTERNET_DRAFT_ARCHIVE_DIR -def submission_file(name, rev, group, format, templatename, author=None, email=None, title=None, year=None, ascii=True): +def submission_file(name_in_doc, name_in_post, group, templatename, author=None, email=None, title=None, year=None, ascii=True): # construct appropriate text draft f = io.open(os.path.join(settings.BASE_DIR, "submit", templatename)) template = f.read() @@ -104,7 +104,7 @@ def submission_file(name, rev, group, format, templatename, author=None, email=N year=year, month=datetime.date.today().strftime("%B"), day=datetime.date.today().strftime("%d"), - name="%s-%s" % (name, rev), + name=name_in_doc, group=group or "", author=author.ascii if ascii else author.name, asciiAuthor=author.ascii, @@ -115,7 +115,7 @@ def submission_file(name, rev, group, format, templatename, author=None, email=N title=title, ) file = StringIO(submission_text) - file.name = "%s-%s.%s" % (name, rev, format) + file.name = name_in_post return file, author def create_draft_submission_with_rev_mismatch(rev='01'): @@ -168,7 +168,7 @@ class SubmitTests(BaseSubmitTestCase): for format in formats: fn = '.'.join((base_filename or 'test_submission', format)) - files[format], __ = submission_file(name, rev, group, format, fn, author=author) + files[format], __ = submission_file(f'{name}-{rev}', f'{name}-{rev}.{format}', group, fn, author=author) r = self.client.post(url, files) if r.status_code != 302: @@ -1599,7 +1599,7 @@ class SubmitTests(BaseSubmitTestCase): def submit_bad_file(self, name, formats): - rev = "" + rev = "00" group = None # break early in case of missing configuration @@ -1614,7 +1614,7 @@ class SubmitTests(BaseSubmitTestCase): # submit files = {} for format in formats: - files[format], author = submission_file(name, rev, group, "bad", "test_submission.bad") + files[format], author = submission_file(f'{name}-{rev}', f'{name}-{rev}.bad', group, "test_submission.bad") r = self.client.post(url, files) @@ -1625,16 +1625,15 @@ class SubmitTests(BaseSubmitTestCase): return r, q, m - def submit_bad_doc_name_with_ext(self, name, formats): + def submit_bad_doc_name_with_ext(self, name_in_doc, name_in_post, formats): group = None url = urlreverse('ietf.submit.views.upload_submission') # submit files = {} for format in formats: - rev = '00.%s' % format - files[format], author = submission_file(name, rev, group, format, "test_submission.%s" % format) - files[format].name = "%s-%s.%s" % (name, 00, format) + files[format], author = submission_file(name_in_doc, name_in_post, group, "test_submission.%s" % format) + files[format].name = name_in_post r = self.client.post(url, files) @@ -1652,10 +1651,15 @@ class SubmitTests(BaseSubmitTestCase): self.assertIn('Expected an TXT file of type "text/plain"', m) self.assertIn('document does not contain a legitimate name', m) - def test_submit_bad_doc_name_txt(self): - r, q, m = self.submit_bad_doc_name_with_ext("draft-foo.dot-bar", ["txt"]) + def test_submit_bad_doc_name(self): + r, q, m = self.submit_bad_doc_name_with_ext(name_in_doc="draft-foo.dot-bar", name_in_post="draft-foo.dot-bar", formats=["txt"]) self.assertIn('contains a disallowed character with byte code: 46', m) - r, q, m = self.submit_bad_doc_name_with_ext("draft-foo-bar", ["xml"]) + # This actually is allowed by the existing code. A significant rework of the validation mechanics is needed. + # r, q, m = self.submit_bad_doc_name_with_ext(name_in_doc="draft-foo-bar-00.txt", name_in_post="draft-foo-bar-00.txt", formats=["txt"]) + # self.assertIn('Did you include a filename extension in the name by mistake?', m) + r, q, m = self.submit_bad_doc_name_with_ext(name_in_doc="draft-foo-bar-00.xml", name_in_post="draft-foo-bar-00.xml", formats=["xml"]) + self.assertIn('Did you include a filename extension in the name by mistake?', m) + r, q, m = self.submit_bad_doc_name_with_ext(name_in_doc="../malicious-name-in-content-00", name_in_post="../malicious-name-in-post-00.xml", formats=["xml"]) self.assertIn('Did you include a filename extension in the name by mistake?', m) def test_submit_bad_file_xml(self): @@ -1692,7 +1696,7 @@ class SubmitTests(BaseSubmitTestCase): fn = os.path.join(dir, "%s-%s.%s" % (name, rev, format)) with io.open(fn, 'w') as f: f.write("a" * 2000) - files[format], author = submission_file(name, rev, group, format, "test_submission.%s" % format) + files[format], author = submission_file(f'{name}-{rev}', f'{name}-{rev}.{format}', group, "test_submission.%s" % format) r = self.client.post(url, files) self.assertEqual(r.status_code, 200) @@ -1717,7 +1721,7 @@ class SubmitTests(BaseSubmitTestCase): user = UserFactory(first_name="Jörgen", last_name="Nilsson") author = PersonFactory(user=user) - file, __ = submission_file(name, rev, group, "txt", "test_submission.nonascii", author=author, ascii=False) + file, __ = submission_file(f'{name}-{rev}', f'{name}-{rev}.txt', group, "test_submission.nonascii", author=author, ascii=False) files = {"txt": file } r = self.client.post(url, files) @@ -1738,7 +1742,7 @@ class SubmitTests(BaseSubmitTestCase): for e in author.email_set.all(): e.delete() - files = {"txt": submission_file(name, rev, group, "txt", "test_submission.txt", author=author, ascii=True)[0] } + files = {"txt": submission_file(f'{name}-{rev}', f'{name}-{rev}.txt', group, "test_submission.txt", author=author, ascii=True)[0] } # submit url = urlreverse('ietf.submit.views.upload_submission') @@ -1762,7 +1766,7 @@ class SubmitTests(BaseSubmitTestCase): email.address = '@bad.email' email.save() - files = {"xml": submission_file(name, rev, group, "xml", "test_submission.xml", author=author, ascii=False)[0] } + files = {"xml": submission_file(f'{name}-{rev}',f'{name}-{rev}.xml', group, "test_submission.xml", author=author, ascii=False)[0] } # submit url = urlreverse('ietf.submit.views.upload_submission') @@ -1782,7 +1786,7 @@ class SubmitTests(BaseSubmitTestCase): group = None # submit - files = {"txt": submission_file(name, rev, group, "txt", "test_submission_invalid_yang.txt")[0] } + files = {"txt": submission_file(f'{name}-{rev}', f'{name}-{rev}.txt', group, "test_submission_invalid_yang.txt")[0] } url = urlreverse('ietf.submit.views.upload_submission') r = self.client.post(url, files) @@ -2673,7 +2677,7 @@ Subject: test # submit files = {} for format in formats: - files[format], author = submission_file(name, rev, group, format, "test_submission.%s" % format) + files[format], author = submission_file(f'{name}-{rev}', f'{name}-{rev}.{format}', group, "test_submission.%s" % format) r = self.client.post(url, files) if r.status_code != 302: @@ -2736,7 +2740,7 @@ class ApiSubmitTests(BaseSubmitTestCase): email = author.user.username # submit data = {} - data['xml'], author = submission_file(name, rev, group, 'xml', "test_submission.xml", author=author, email=email, title=title, year=year) + data['xml'], author = submission_file(f'{name}-{rev}', f'{name}-{rev}.xml', group, "test_submission.xml", author=author, email=email, title=title, year=year) data['user'] = email r = self.client.post(url, data) return r, author, name @@ -2857,7 +2861,7 @@ class RefsTests(BaseSubmitTestCase): def test_draft_refs_identification(self): group = None - file, __ = submission_file('draft-some-subject', '00', group, 'txt', "test_submission.txt", ) + file, __ = submission_file('draft-some-subject-00', 'draft-some-subject-00.txt', group, "test_submission.txt", ) draft = PlaintextDraft(file.read(), file.name) refs = draft.get_refs() self.assertEqual(refs['rfc2119'], 'norm') diff --git a/ietf/submit/utils.py b/ietf/submit/utils.py index 495759dd6..f3f21e4fd 100644 --- a/ietf/submit/utils.py +++ b/ietf/submit/utils.py @@ -125,6 +125,7 @@ def validate_submission_name(name): if '.' in name: msg += " Did you include a filename extension in the name by mistake?" return msg + return None def validate_submission_rev(name, rev): if not rev: diff --git a/ietf/utils/tests.py b/ietf/utils/tests.py index 2d0bc1048..874a71fc3 100644 --- a/ietf/utils/tests.py +++ b/ietf/utils/tests.py @@ -422,7 +422,7 @@ class PlaintextDraftTests(TestCase): def setUp(self): super().setUp() - file,_ = submission_file(name='draft-test-draft-class',rev='00',format='txt',templatename='test_submission.txt',group=None) + file,_ = submission_file(name_in_doc='draft-test-draft-class-00', name_in_post='draft-test-draft-class-00.txt',templatename='test_submission.txt',group=None) self.draft = PlaintextDraft(text=file.getvalue(), source='draft-test-draft-class-00.txt', name_from_source=False) def test_get_status(self):