Skip to content

Commit

Permalink
Linting and hardening HTTP requests
Browse files Browse the repository at this point in the history
  • Loading branch information
@glpatcern
glpatcern committed Nov 24, 2023
1 parent 9a5a7a8 commit 2515281
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 87 deletions.
104 changes: 57 additions & 47 deletions src/bridge/codimd.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ def init(_appurl, _appinturl, _apikey):
appurl = _appinturl
try:
# CodiMD integrates Prometheus metrics, let's probe if they exist
res = requests.head(appurl + '/metrics/codimd', verify=sslverify)
res = requests.head(appurl + '/metrics/codimd', verify=sslverify, timeout=10)
if res.status_code != http.client.OK:
log.error(f'msg="The provided URL does not seem to be a CodiMD instance" appurl="{appurl}"')
raise AppFailure
log.info(f'msg="Successfully connected to CodiMD" appurl="{appurl}"')
except requests.exceptions.ConnectionError as e:
except requests.exceptions.RequestException as e:
log.error(f'msg="Exception raised attempting to connect to CodiMD" exception="{e}"')
raise AppFailure

Expand Down Expand Up @@ -77,55 +77,60 @@ def getredirecturl(viewmode, wopisrc, acctok, docid, filename, displayname, reva

def _unzipattachments(inputbuf):
'''Unzip the given input buffer uploading the content to CodiMD and return the contained .md file'''
mddoc = None
try:
inputzip = zipfile.ZipFile(io.BytesIO(inputbuf), compression=zipfile.ZIP_STORED)
for zipinfo in inputzip.infolist():
fname = zipinfo.filename
log.debug(f'msg="Extracting attachment" name="{fname}"')
if os.path.splitext(fname)[1] == '.md':
mddoc = inputzip.read(zipinfo)
else:
# first check if the file already exists in CodiMD:
res = requests.head(appurl + '/uploads/' + fname, verify=sslverify, timeout=10)
if res.status_code == http.client.OK and int(res.headers['Content-Length']) == zipinfo.file_size:
# yes (assume that hashed filename AND size matching is a good enough content match!)
log.debug(f'msg="Skipped existing attachment" filename="{fname}"')
continue
# check for collision
if res.status_code == http.client.OK:
log.warning(f'msg="Attachment collision detected" filename="{fname}"')
# append a random letter to the filename
name, ext = os.path.splitext(fname)
fname = name + '_' + chr(randint(65, 65+26)) + ext
# and replace its reference in the document (this creates a copy of the doc, not very efficient)
mddoc = mddoc.replace(bytes(zipinfo.filename), bytes(fname))
# OK, let's upload
log.debug(f'msg="Pushing attachment" filename="{fname}"')
res = requests.post(appurl + '/uploadimage', params={'generateFilename': 'false'},
files={'image': (fname, inputzip.read(zipinfo))}, verify=sslverify, timeout=10)
if res.status_code != http.client.OK:
log.error('msg="Failed to push included file" filename="%s" httpcode="%d"' % (fname, res.status_code))
if mddoc:
# for backwards compatibility, drop the hardcoded reverse proxy paths if found in the document
mddoc = mddoc.replace(b'/byoa/codimd/', b'/')
return mddoc
except zipfile.BadZipFile as e:
raise AppFailure('The file is not in the expected zipped format')
mddoc = None
for zipinfo in inputzip.infolist():
fname = zipinfo.filename
log.debug(f'msg="Extracting attachment" name="{fname}"')
if os.path.splitext(fname)[1] == '.md':
mddoc = inputzip.read(zipinfo)
else:
# first check if the file already exists in CodiMD:
res = requests.head(appurl + '/uploads/' + fname, verify=sslverify)
if res.status_code == http.client.OK and int(res.headers['Content-Length']) == zipinfo.file_size:
# yes (assume that hashed filename AND size matching is a good enough content match!)
log.debug(f'msg="Skipped existing attachment" filename="{fname}"')
continue
# check for collision
if res.status_code == http.client.OK:
log.warning(f'msg="Attachment collision detected" filename="{fname}"')
# append a random letter to the filename
name, ext = os.path.splitext(fname)
fname = name + '_' + chr(randint(65, 65+26)) + ext
# and replace its reference in the document (this creates a copy of the doc, not very efficient)
mddoc = mddoc.replace(bytes(zipinfo.filename), bytes(fname))
# OK, let's upload
log.debug(f'msg="Pushing attachment" filename="{fname}"')
res = requests.post(appurl + '/uploadimage', params={'generateFilename': 'false'},
files={'image': (fname, inputzip.read(zipinfo))}, verify=sslverify)
if res.status_code != http.client.OK:
log.error('msg="Failed to push included file" filename="%s" httpcode="%d"' % (fname, res.status_code))
if mddoc:
# for backwards compatibility, drop the hardcoded reverse proxy paths if found in the document
mddoc = mddoc.replace(b'/byoa/codimd/', b'/')
return mddoc
log.warn(f'msg="File is not in a valid zip format" exception="{e}"')
raise AppFailure('The file is not in the expected zipped format') from e
except requests.exceptions.RequestException as e:
log.error(f'msg="Exception raised attempting to connect to CodiMD" exception="{e}"')
raise AppFailure('Failed to connect to CodiMD') from e


def _fetchfromcodimd(wopilock, acctok):
'''Fetch a given document from from CodiMD, raise AppFailure in case of errors'''
try:
res = requests.get(appurl + ('/' if wopilock['doc'][0] != '/' else '') + wopilock['doc'] + '/download', verify=sslverify)
res = requests.get(appurl + ('/' if wopilock['doc'][0] != '/' else '') + wopilock['doc'] + '/download',
verify=sslverify, timeout=10)
if res.status_code != http.client.OK:
log.error('msg="Unable to fetch document from CodiMD" token="%s" response="%d: %s"' %
(acctok[-20:], res.status_code, res.content.decode()[:50]))
raise AppFailure
return res.content
except requests.exceptions.ConnectionError as e:
except requests.exceptions.RequestException as e:
log.error(f'msg="Exception raised attempting to connect to CodiMD" exception="{e}"')
raise AppFailure
raise AppFailure('Failed to connect to CodiMD') from e


def loadfromstorage(filemd, wopisrc, acctok, docid):
Expand All @@ -149,7 +154,8 @@ def loadfromstorage(filemd, wopisrc, acctok, docid):
allow_redirects=False,
params={'mode': 'locked'},
headers={'Content-Type': 'text/markdown'},
verify=sslverify)
verify=sslverify,
timeout=10)
if res.status_code == http.client.REQUEST_ENTITY_TOO_LARGE:
log.error(f'msg="File is too large to be edited in CodiMD" token="{acctok[-20:]}"')
raise AppFailure(TOOLARGE)
Expand All @@ -164,7 +170,8 @@ def loadfromstorage(filemd, wopisrc, acctok, docid):
# reserve the given docid in CodiMD via a HEAD request
res = requests.head(appurl + '/' + docid,
allow_redirects=False,
verify=sslverify)
verify=sslverify,
timeout=10)
if res.status_code not in (http.client.OK, http.client.FOUND):
log.error('msg="Unable to reserve note hash in CodiMD" token="%s" response="%d"' %
(acctok[-20:], res.status_code))
Expand All @@ -180,7 +187,8 @@ def loadfromstorage(filemd, wopisrc, acctok, docid):
# push the document to CodiMD with the update API
res = requests.put(appurl + '/api/notes/' + docid,
json={'content': mddoc.decode()},
verify=sslverify)
verify=sslverify,
timeout=10)
if res.status_code == http.client.FORBIDDEN:
# the file got unlocked because of no activity, yet some user is there: let it go
log.warning(f'msg="Document was being edited in CodiMD, redirecting user" token="{acctok[-20:]}"')
Expand All @@ -193,13 +201,13 @@ def loadfromstorage(filemd, wopisrc, acctok, docid):
raise AppFailure

log.info(f'msg="Pushed document to CodiMD" docid="{docid}" token="{acctok[-20:]}"')
except requests.exceptions.ConnectionError as e:
except requests.exceptions.RequestException as e:
log.error(f'msg="Exception raised attempting to connect to CodiMD" exception="{e}"')
raise AppFailure
raise AppFailure from e
except UnicodeDecodeError as e:
log.warning(f'msg="Invalid UTF-8 content found in file" exception="{e}"')
raise AppFailure('File contains an invalid UTF-8 character, was it corrupted? ' +
'Please fix it in a regular editor before opening it in CodiMD.')
'Please fix it in a regular editor before opening it in CodiMD.') from e
# generate and return a WOPI lock structure for this document
return wopic.generatelock(docid, filemd, mddoc, acctok, False)

Expand All @@ -213,10 +221,12 @@ def _getattachments(mddoc, docfilename, forcezip=False):
response = None
for attachment in upload_re.findall(mddoc):
log.debug(f'msg="Fetching attachment" url="{attachment}"')
res = requests.get(appurl + attachment, verify=sslverify)
if res.status_code != http.client.OK:
log.error('msg="Failed to fetch included file, skipping" path="%s" response="%d"' % (
attachment, res.status_code))
try:
res = requests.get(appurl + attachment, verify=sslverify, timeout=10)
if res.status_code != http.client.OK:
raise ValueError(res.status_code)
except (requests.exceptions.RequestException, ValueError) as e:
log.error(f'msg="Failed to fetch included file, skipping" path="{attachment}" type="{type(e)}" error="{e}"')
# also notify the user
response = wopic.jsonify('Failed to include a referenced picture in the saved file'), http.client.NOT_FOUND
continue
Expand Down
33 changes: 18 additions & 15 deletions src/bridge/etherpad.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,19 +47,19 @@ def _apicall(method, params, data=None, acctok=None, raiseonnonzerocode=True):
'''Generic method to call the Etherpad REST API'''
params['apikey'] = apikey
try:
res = requests.post(appurl + '/api/1/' + method, params=params, data=data, verify=sslverify)
res = requests.post(appurl + '/api/1/' + method, params=params, data=data, verify=sslverify, timeout=10)
if res.status_code != http.client.OK:
log.error('msg="Failed to call Etherpad" method="%s" token="%s" response="%d: %s"' %
(method, acctok[-20:] if acctok else 'N/A', res.status_code, res.content.decode()))
raise AppFailure
except requests.exceptions.ConnectionError as e:
raise AppFailure('Failed to connect to Etherpad')
except requests.exceptions.RequestException as e:
log.error(f'msg="Exception raised attempting to connect to Etherpad" method="{method}" exception="{e}"')
raise AppFailure
raise AppFailure('Failed to connect to Etherpad') from e
res = res.json()
if res['code'] != 0 and raiseonnonzerocode:
log.error('msg="Error response from Etherpad" method="%s" token="%s" response="%s"' %
(method, acctok[-20:] if acctok else 'N/A', res['message']))
raise AppFailure
raise AppFailure('Error response from Etherpad')
log.debug('msg="Called Etherpad API" method="%s" token="%s" result="%s"' %
(method, acctok[-20:] if acctok else 'N/A', res))
return res
Expand All @@ -77,15 +77,16 @@ def getredirecturl(viewmode, wopisrc, acctok, docid, _filename, displayname, _re
res = requests.post(appurl + '/setEFSSMetadata',
params={'padID': docid, 'wopiSrc': urlparse.quote_plus(wopisrc),
'accessToken': acctok, 'apikey': apikey},
verify=sslverify)
verify=sslverify,
timeout=10)
if res.status_code != http.client.OK or res.json()['code'] != 0:
log.error('msg="Failed to call Etherpad" method="setEFSSMetadata" token="%s" response="%d: %s"' %
(acctok[-20:], res.status_code, res.content.decode().replace('"', "'")))
raise AppFailure
raise AppFailure('Error response from Etherpad')
log.debug(f'msg="Called Etherpad" method="setEFSSMetadata" token="{acctok[-20:]}"')
except requests.exceptions.ConnectionError as e:
except requests.exceptions.RequestException as e:
log.error(f'msg="Exception raised attempting to connect to Etherpad" method="setEFSSMetadata" exception="{e}"')
raise AppFailure
raise AppFailure('Failed to connect to Etherpad') from e

# return the URL to the pad for editing (a PREVIEW viewmode is not supported)
return appexturl + f'/p/{docid}?userName={urlparse.quote_plus(displayname)}'
Expand Down Expand Up @@ -116,15 +117,16 @@ def loadfromstorage(filemd, wopisrc, acctok, docid):
res = requests.post(appurl + '/p/' + docid + '/import',
files={'file': (docid + '.etherpad', epfile, 'application/json')},
params={'apikey': apikey},
verify=sslverify)
verify=sslverify,
timeout=10)
if res.status_code != http.client.OK:
log.error('msg="Unable to push document to Etherpad" token="%s" padid="%s" response="%d: %s"' %
(acctok[-20:], docid, res.status_code, res.content.decode()))
raise AppFailure
raise AppFailure('Error response from Etherpad')
log.info(f'msg="Pushed document to Etherpad" padid="{docid}" token="{acctok[-20:]}"')
except requests.exceptions.ConnectionError as e:
except requests.exceptions.RequestException as e:
log.error(f'msg="Exception raised attempting to connect to Etherpad" method="import" exception="{e}"')
raise AppFailure
raise AppFailure('Failed to connect to Etherpad') from e
# generate and return a WOPI lock structure for this document
return wopic.generatelock(docid, filemd, epfile, acctok, False)

Expand All @@ -137,13 +139,14 @@ def _fetchfrometherpad(wopilock, acctok):
try:
# this operation does not use the API (and it is NOT protected by the API key!), so we use a plain GET
res = requests.get(appurl + '/p' + wopilock['doc'] + '/export/etherpad',
verify=sslverify)
verify=sslverify,
timeout=10)
if res.status_code != http.client.OK:
log.error('msg="Unable to fetch document from Etherpad" token="%s" response="%d: %s"' %
(acctok[-20:], res.status_code, res.content.decode()[:50]))
raise AppFailure
return res.content
except requests.exceptions.ConnectionError as e:
except requests.exceptions.RequestException as e:
log.error(f'msg="Exception raised attempting to connect to Etherpad" exception="{e}"')
raise AppFailure

Expand Down
10 changes: 5 additions & 5 deletions src/bridge/wopiclient.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,11 @@ def request(wopisrc, acctok, method, contents=None, headers=None):
log.debug('msg="Calling WOPI" url="%s" headers="%s" acctok="%s" ssl="%s"' %
(wopiurl, headers, acctok[-20:], sslverify))
if method == 'GET':
return requests.get(f'{wopiurl}?access_token={acctok}', verify=sslverify)
return requests.get(f'{wopiurl}?access_token={acctok}', verify=sslverify, timeout=10)
if method == 'POST':
return requests.post(f'{wopiurl}?access_token={acctok}', verify=sslverify,
headers=headers, data=contents)
except (requests.exceptions.ConnectionError, IOError) as e:
headers=headers, data=contents, timeout=10)
except (requests.exceptions.RequestException, IOError) as e:
log.error(f'msg="Unable to contact WOPI" wopiurl="{wopiurl}" acctok="{acctok}" response="{e}"')
res = Response()
res.status_code = http.client.INTERNAL_SERVER_ERROR
Expand Down Expand Up @@ -90,7 +90,7 @@ def getlock(wopisrc, acctok):
return json.loads(res.headers['X-WOPI-Lock'])
except (ValueError, KeyError, json.decoder.JSONDecodeError) as e:
log.warning(f'msg="Missing or malformed WOPI lock" exception="{type(e)}: {e}"')
raise InvalidLock(e)
raise InvalidLock(e) from e


def _getheadersforrelock(acctok, wopilock, digest, toclose):
Expand Down Expand Up @@ -125,7 +125,7 @@ def refreshlock(wopisrc, acctok, wopilock, digest=None, toclose=None):
except json.decoder.JSONDecodeError as e:
log.error('msg="Got unresolvable conflict in RefreshLock" url="%s" previouslock="%s" error="%s"' %
(wopisrc, res.headers.get('X-WOPI-Lock'), e))
raise InvalidLock('Found existing malformed lock on refreshlock')
raise InvalidLock('Found existing malformed lock on refreshlock') from e
if toclose:
# merge toclose token lists
for t in currlock['tocl']:
Expand Down
8 changes: 4 additions & 4 deletions src/core/cs3iface.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def init(inconfig, inlog):
grpc.channel_ready_future(ch).result(timeout=10)
except grpc.FutureTimeoutError as e:
log.error('msg="Failed to connect to Reva via GRPC" error="%s"' % e)
raise IOError(e)
raise IOError(e) from e
ctx['cs3gw'] = cs3gw_grpc.GatewayAPIStub(ch)


Expand Down Expand Up @@ -412,7 +412,7 @@ def readfile(endpoint, filepath, userid, lockid):
'x-access-token': userid,
'x-reva-transfer': protocol.token # needed if the downloads pass through the data gateway in reva
}
fileget = requests.get(url=protocol.download_endpoint, headers=headers, verify=ctx['ssl_verify'], timeout=30, stream=True)
fileget = requests.get(url=protocol.download_endpoint, headers=headers, verify=ctx['ssl_verify'], timeout=10, stream=True)
except requests.exceptions.RequestException as e:
log.error(f'msg="Exception when downloading file from Reva" reason="{e}"')
yield IOError(e)
Expand Down Expand Up @@ -466,10 +466,10 @@ def writefile(endpoint, filepath, userid, content, lockmd, islock=False):
'Upload-Length': size,
'x-reva-transfer': protocol.token # needed if the uploads pass through the data gateway in reva
}
putres = requests.put(url=protocol.upload_endpoint, data=content, headers=headers, verify=ctx['ssl_verify'], timeout=30)
putres = requests.put(url=protocol.upload_endpoint, data=content, headers=headers, verify=ctx['ssl_verify'], timeout=10)
except requests.exceptions.RequestException as e:
log.error(f'msg="Exception when uploading file to Reva" reason="{e}"')
raise IOError(e)
raise IOError(e) from e
if putres.status_code == http.client.UNAUTHORIZED:
log.warning(f'msg="Access denied uploading file to Reva" reason="{putres.reason}"')
raise IOError(common.ACCESS_ERROR)
Expand Down
Loading

0 comments on commit 2515281

Please sign in to comment.