From 26605049a48d75de6a47ae2cc3ad66978e772a3b Mon Sep 17 00:00:00 2001 From: Bruce Martin Date: Wed, 25 Mar 2020 17:14:52 -0600 Subject: [PATCH] Various hardening to REST routes (#1293) * URL reweriting for static * request size limits * improve quotas, make tests work * remove debugging code * pass limits to front-end * fix renaming boggle --- server/common/app_config.py | 22 ++++++++++++++++ server/common/errors.py | 8 ++++++ server/common/rest.py | 25 +++++++++++++------ server/data_anndata/anndata_adaptor.py | 8 ++++++ server/data_common/data_adaptor.py | 21 +++++++++++++++- server/data_cxg/cxg_adaptor.py | 10 ++++++++ server/eb/.ebextensions/static_rewrite.config | 11 ++++++++ server/test/test_anndata_adaptor.py | 2 ++ server/test/test_api.py | 15 ++++------- server/test/test_nan_anndata_adaptor.py | 2 ++ server/test/test_nan_rest.py | 3 ++- 11 files changed, 107 insertions(+), 20 deletions(-) create mode 100644 server/eb/.ebextensions/static_rewrite.config diff --git a/server/common/app_config.py b/server/common/app_config.py index a3b745ef..39fa637f 100644 --- a/server/common/app_config.py +++ b/server/common/app_config.py @@ -21,6 +21,18 @@ DEFAULT_SERVER_PORT = int(environ.get("CXG_SERVER_PORT", "5005")) # anything bigger than this will generate a special message BIG_FILE_SIZE_THRESHOLD = 100 * 2 ** 20 # 100MB +""" Default limits for requests """ +Default_Limits = { + # Max number of columns that may be requested for /annotations or /data routes. + # This is a simplistic means of preventing excess resource consumption (eg, + # requesting the entire X matrix in one request) or other DoS style attacks/errors. + # Set to None to disable check. + "column_request_max": 32, + # Max number of cells that will be accepted for differential expression. + # Set to None to disable the check. + "diffexp_cellcount_max": None, # None is disabled +} + class AppFeature(object): def __init__(self, path, available=False, method="POST", extra={}): @@ -78,6 +90,9 @@ class AppConfig(object): except KeyError as e: raise ConfigurationError(f"Unexpected config: {str(e)}") + # Used for various limits, eg, size of requests. Not currently configurable. + self.limits = Default_Limits + # The annotation object is created during complete_config and stored here. self.user_annotations = None @@ -425,5 +440,12 @@ class AppConfig(object): config["library_versions"] = library_versions config["links"] = links config["parameters"] = parameters + config["limits"] = self.limits return c + + def exceeds_limit(self, limit_name, value): + limit_value = self.limits.get(limit_name, None) + if limit_value is None: # disabled + return False + return value > limit_value diff --git a/server/common/errors.py b/server/common/errors.py index 6ef18c57..cbc9e08c 100644 --- a/server/common/errors.py +++ b/server/common/errors.py @@ -68,3 +68,11 @@ class ConfigurationError(Exception): """ pass + + +class ExceedsLimitError(Exception): + """ + Raised when an HTTP request exceeds a limit/quota + """ + + pass diff --git a/server/common/rest.py b/server/common/rest.py index 111aca2d..0dbf84af 100644 --- a/server/common/rest.py +++ b/server/common/rest.py @@ -9,6 +9,7 @@ from server.common.errors import ( JSONEncodingValueError, PrepareError, DisabledFeatureError, + ExceedsLimitError, ) import json @@ -54,9 +55,13 @@ def config_get(app_config, data_adaptor, annotations): def annotations_obs_get(request, data_adaptor, annotations): fields = request.args.getlist("annotation-name", None) + num_columns_requested = len(data_adaptor.get_obs_keys()) if len(fields) == 0 else len(fields) + if data_adaptor.config.exceeds_limit("column_request_max", num_columns_requested): + return abort(HTTPStatus.BAD_REQUEST) preferred_mimetype = request.accept_mimetypes.best_match(["application/octet-stream"]) if preferred_mimetype != "application/octet-stream": return abort(HTTPStatus.NOT_ACCEPTABLE) + try: labels = None if annotations: @@ -100,9 +105,13 @@ def annotations_obs_put(request, data_adaptor, annotations): def annotations_var_get(request, data_adaptor, annotations): fields = request.args.getlist("annotation-name", None) + num_columns_requested = len(data_adaptor.get_var_keys()) if len(fields) == 0 else len(fields) + if data_adaptor.config.exceeds_limit("column_request_max", num_columns_requested): + return abort(HTTPStatus.BAD_REQUEST) preferred_mimetype = request.accept_mimetypes.best_match(["application/octet-stream"]) if preferred_mimetype != "application/octet-stream": return abort(HTTPStatus.NOT_ACCEPTABLE) + try: labels = None if annotations is not None: @@ -129,7 +138,7 @@ def data_var_put(request, data_adaptor): HTTPStatus.OK, {"Content-Type": "application/octet-stream"}, ) - except FilterError as e: + except (FilterError, ValueError, ExceedsLimitError) as e: return abort_and_log(HTTPStatus.BAD_REQUEST, str(e), include_exc_info=True) @@ -160,7 +169,7 @@ def diffexp_obs_post(request, data_adaptor): try: diffexp = data_adaptor.diffexp_topN(set1_filter, set2_filter, count) return make_response(diffexp, HTTPStatus.OK, {"Content-Type": "application/json"}) - except (ValueError, DisabledFeatureError, FilterError) as e: + except (ValueError, DisabledFeatureError, FilterError, ExceedsLimitError) as e: return abort_and_log(HTTPStatus.BAD_REQUEST, str(e), include_exc_info=True) except JSONEncodingValueError: # JSON encoding failure, usually due to bad data. Just let it ripple up @@ -171,13 +180,13 @@ def diffexp_obs_post(request, data_adaptor): def layout_obs_get(request, data_adaptor): preferred_mimetype = request.accept_mimetypes.best_match(["application/octet-stream"]) + if preferred_mimetype != "application/octet-stream": + return abort(HTTPStatus.NOT_ACCEPTABLE) + try: - if preferred_mimetype == "application/octet-stream": - return make_response( - data_adaptor.layout_to_fbs_matrix(), HTTPStatus.OK, {"Content-Type": "application/octet-stream"} - ) - else: - return abort(HTTPStatus.NOT_ACCEPTABLE) + return make_response( + data_adaptor.layout_to_fbs_matrix(), HTTPStatus.OK, {"Content-Type": "application/octet-stream"} + ) except PrepareError: return abort_and_log( HTTPStatus.NOT_IMPLEMENTED, diff --git a/server/data_anndata/anndata_adaptor.py b/server/data_anndata/anndata_adaptor.py index 4442ff13..3b8c7212 100644 --- a/server/data_anndata/anndata_adaptor.py +++ b/server/data_anndata/anndata_adaptor.py @@ -351,3 +351,11 @@ class AnndataAdaptor(DataAdaptor): def get_obs_columns(self): return self.data.obs.columns + + def get_obs_keys(self): + # return list of keys + return self.data.obs.keys().to_list() + + def get_var_keys(self): + # return list of keys + return self.data.var.keys().to_list() diff --git a/server/data_common/data_adaptor.py b/server/data_common/data_adaptor.py index 4b22264b..5218cc4e 100644 --- a/server/data_common/data_adaptor.py +++ b/server/data_common/data_adaptor.py @@ -6,7 +6,7 @@ from os.path import basename, splitext from server.data_common.fbs.matrix import encode_matrix_fbs from server.common.constants import Axis, DEFAULT_TOP_N -from server.common.errors import FilterError, JSONEncodingValueError +from server.common.errors import FilterError, JSONEncodingValueError, ExceedsLimitError from server.compute.diffexp import diffexp_ttest from server.common.utils import jsonify_numpy from server.common.app_config import AppFeature, AppConfig @@ -92,6 +92,16 @@ class DataAdaptor(metaclass=ABCMeta): def get_obs_columns(self): pass + @abstractmethod + def get_obs_keys(self): + # return list of keys + pass + + @abstractmethod + def get_var_keys(self): + # return list of keys + pass + @abstractmethod def cleanup(self): pass @@ -259,6 +269,10 @@ class DataAdaptor(metaclass=ABCMeta): if obs_selector is not None: raise FilterError("filtering on obs unsupported") + num_columns = self.get_shape()[1] if var_selector is None else np.count_nonzero(var_selector) + if self.config.exceeds_limit("column_request_max", num_columns): + raise ExceedsLimitError("Requested dataframe columns exceed column request limit") + X = self.get_X_array(obs_selector, var_selector) col_idx = np.nonzero([] if var_selector is None else var_selector)[0] return encode_matrix_fbs(X, col_idx=col_idx, row_idx=None) @@ -286,6 +300,11 @@ class DataAdaptor(metaclass=ABCMeta): if top_n is None: top_n = DEFAULT_TOP_N + if self.config.exceeds_limit( + "diffexp_cellcount_max", np.count_nonzero(obs_mask_A) + np.count_nonzero(obs_mask_B) + ): + raise ExceedsLimitError("Diffexp request exceeds max cell count limit") + result = diffexp_ttest(self, obs_mask_A, obs_mask_B, top_n, self.config.diffexp__lfc_cutoff) try: diff --git a/server/data_cxg/cxg_adaptor.py b/server/data_cxg/cxg_adaptor.py index 3db73890..9a091365 100644 --- a/server/data_cxg/cxg_adaptor.py +++ b/server/data_cxg/cxg_adaptor.py @@ -234,6 +234,16 @@ class CxgAdaptor(DataAdaptor): col_names = [attr.name for attr in schema] return pd.Index(col_names) + def get_obs_keys(self): + obs = self.open_array("obs") + schema = obs.schema + return [attr.name for attr in schema] + + def get_var_keys(self): + var = self.open_array("var") + schema = var.schema + return [attr.name for attr in schema] + # function to get the embedding # this function to iterate through embeddings. def get_embedding_names(self): diff --git a/server/eb/.ebextensions/static_rewrite.config b/server/eb/.ebextensions/static_rewrite.config new file mode 100644 index 00000000..f8cc8668 --- /dev/null +++ b/server/eb/.ebextensions/static_rewrite.config @@ -0,0 +1,11 @@ +# Configure URL rewriting from //static/* to /static/* +files: + "/etc/httpd/conf.d/static_rewrite.conf": + mode: "000644" + owner: root + group: root + content: | + RewriteEngine On + RewriteRule "^(.*)/static/(.*)" "/static/$2" [PT] + RewriteRule "^/favicon.jpg" "/static/img/favicon.jpg" [PT] + \ No newline at end of file diff --git a/server/test/test_anndata_adaptor.py b/server/test/test_anndata_adaptor.py index 6848da67..7e623623 100644 --- a/server/test/test_anndata_adaptor.py +++ b/server/test/test_anndata_adaptor.py @@ -44,6 +44,8 @@ class AdaptorTest(unittest.TestCase): } config = AppConfig() config.update(**args) + for k in config.limits.keys(): + config.limits[k] = None config.complete_config() self.data = AnndataAdaptor(DataLocator(self.data_locator), config) diff --git a/server/test/test_api.py b/server/test/test_api.py index 14ca30a5..51c83751 100644 --- a/server/test/test_api.py +++ b/server/test/test_api.py @@ -193,7 +193,10 @@ class EndPoints(object): endpoint = f"data/var" url = f"{self.URL_BASE}{endpoint}" result = self.session.put(url) - self.assertEqual(result.status_code, HTTPStatus.OK) + self.assertEqual(result.status_code, HTTPStatus.BAD_REQUEST) + + filter = {"filter": {"var": {"index": [0, 1, 4]}}} + result = self.session.put(url, json=filter) self.assertEqual(result.headers["Content-Type"], "application/octet-stream") def test_data_put_fbs(self): @@ -201,15 +204,7 @@ class EndPoints(object): url = f"{self.URL_BASE}{endpoint}" header = {"Accept": "application/octet-stream"} result = self.session.put(url, headers=header) - self.assertEqual(result.status_code, HTTPStatus.OK) - self.assertEqual(result.headers["Content-Type"], "application/octet-stream") - df = decode_fbs.decode_matrix_FBS(result.content) - self.assertEqual(df["n_rows"], 2638) - self.assertEqual(df["n_cols"], 1838) - self.assertIsNotNone(df["columns"]) - self.assertListEqual(df["col_idx"].tolist(), []) - self.assertIsNone(df["row_idx"]) - self.assertEqual(len(df["columns"]), df["n_cols"]) + self.assertEqual(result.status_code, HTTPStatus.BAD_REQUEST) def test_data_put_filter_fbs(self): endpoint = f"data/var" diff --git a/server/test/test_nan_anndata_adaptor.py b/server/test/test_nan_anndata_adaptor.py index 6ce560ab..861c2776 100644 --- a/server/test/test_nan_anndata_adaptor.py +++ b/server/test/test_nan_anndata_adaptor.py @@ -24,6 +24,8 @@ class NaNTest(unittest.TestCase): config.update(**self.args) locator = DataLocator("test/test_datasets/nan.h5ad") config.update(single_dataset__datapath=locator.path) + for k in config.limits.keys(): + config.limits[k] = None config.complete_config() with warnings.catch_warnings(): diff --git a/server/test/test_nan_rest.py b/server/test/test_nan_rest.py index 15995865..8e958eeb 100644 --- a/server/test/test_nan_rest.py +++ b/server/test/test_nan_rest.py @@ -47,7 +47,8 @@ class WithNaNs(unittest.TestCase): def test_data(self): endpoint = "data/var" url = f"{URL_BASE}{endpoint}" - result = self.session.put(url) + filter = {"filter": {"var": {"index": [[0, 20]]}}} + result = self.session.put(url, json=filter) self.assertEqual(result.status_code, HTTPStatus.OK) self.assertEqual(result.headers["Content-Type"], "application/octet-stream") df = decode_fbs.decode_matrix_FBS(result.content)