From 8e2493476ae311f5438f7bfd7d91b6b8fbab1d6b Mon Sep 17 00:00:00 2001 From: Alejandro Roiz Walss Date: Wed, 24 Jul 2024 08:28:04 -0600 Subject: [PATCH] Revert "Further Sanitize User Input (#425)" This reverts commit 0bdd433dafbb6bc91a9054cab043dbcf588814e1. --- VERSION | 2 +- confidant/routes/credentials.py | 71 ++++--------------- .../unit/confidant/routes/credentials_test.py | 28 -------- 3 files changed, 14 insertions(+), 87 deletions(-) diff --git a/VERSION b/VERSION index 54c95af1..cc81d718 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -6.5.7 +6.5.5 diff --git a/confidant/routes/credentials.py b/confidant/routes/credentials.py index 4bbf89e3..13434add 100644 --- a/confidant/routes/credentials.py +++ b/confidant/routes/credentials.py @@ -594,8 +594,9 @@ def create_credential(): ''' with stats.timer('create_credential'): if not acl_module_check(resource_type='credential', action='create'): - msg = f"{authnz.get_logged_in_user()}" - msg += "does not have access to create credentials" + msg = "{} does not have access to create credentials".format( + authnz.get_logged_in_user() + ) error_msg = {'error': msg} return jsonify(error_msg), 403 @@ -620,7 +621,7 @@ def create_credential(): for cred in Credential.data_type_date_index.query( 'credential', filter_condition=Credential.name == data['name']): # Conflict, the name already exists - msg = f'Name already exists. See id: {cred.id}' + msg = 'Name already exists. See id: {0}'.format(cred.id) return jsonify({'error': msg, 'reference': cred.id}), 409 # Generate an initial stable ID to allow name changes id = str(uuid.uuid4()).replace('-', '') @@ -635,20 +636,13 @@ def create_credential(): credential_pairs = cipher.encrypt(credential_pairs) last_rotation_date = misc.utcnow() - metadata = data.get('metadata', {}) - for key, value in metadata.items(): - value = escape(value) - metadata[key] = value - - data['documentation'] = escape(data.get('documentation')) - sanitized_name = escape(data['name']) cred = Credential( - id=f'{id}-{revision}', + id='{0}-{1}'.format(id, revision), data_type='archive-credential', name=sanitized_name, credential_pairs=credential_pairs, - metadata=metadata, + metadata=data.get('metadata'), revision=revision, enabled=data.get('enabled'), data_key=data_key['ciphertext'], @@ -803,8 +797,10 @@ def update_credential(id): if not acl_module_check(resource_type='credential', action='update', resource_id=id): - msg = f"{authnz.get_logged_in_user()}" - msg += f"does not have access to update credential {id}" + msg = "{} does not have access to update credential {}".format( + authnz.get_logged_in_user(), + id + ) error_msg = {'error': msg, 'reference': id} return jsonify(error_msg), 403 @@ -820,34 +816,6 @@ def update_credential(id): if not isinstance(data.get('metadata', {}), dict): return jsonify({'error': 'metadata must be a dict'}), 400 - # We check for a name change and ensure it doesn't conflict with an - # existing credential and to ensure we don't escape the name if it - # hasn't changed - if data.get('name') != _cred.name: - data['name'] = escape(data.get('name')) - for cred in Credential.data_type_date_index.query( - 'credential', - filter_condition=Credential.name == data['name']): - # Conflict, the name already exists - msg = f'Name already exists. See id: {cred.id}' - return jsonify({'error': msg, 'reference': cred.id}), 409 - - # Escape metadata values by checking for new metadata keys and values - # to ensure we don't escape values that haven't changed - if data.get('metadata') != _cred.metadata: - new_metadata = { - key: value - for key, value in data.get('metadata', {}).items() - if key not in _cred.metadata or - value != _cred.metadata.get(key) - } - for key, value in new_metadata.items(): - value = escape(value) - data['metadata'][key] = value - - if data.get('documentation') != _cred.documentation: - data['documentation'] = escape(data.get('documentation')) - update = { 'name': data.get('name', _cred.name), 'last_rotation_date': _cred.last_rotation_date, @@ -906,19 +874,6 @@ def update_credential(id): # this is a new credential pair and update last_rotation_date if credential_pairs != _cred.decrypted_credential_pairs: update['last_rotation_date'] = misc.utcnow() - - # We escape credential pairs by checking for new credential - # pairs and values to ensure we don't escape values that haven't - # changed - new_credential_pairs = { - key: value - for key, value in credential_pairs.items() - if key not in _cred.decrypted_credential_pairs or - value != _cred.decrypted_credential_pairs.get(key) - } - for key, value in new_credential_pairs.items(): - value = escape(value) - credential_pairs[key] = value data_key = keymanager.create_datakey(encryption_context={'id': id}) cipher = CipherManager(data_key['plaintext'], version=2) update['credential_pairs'] = cipher.encrypt( @@ -928,7 +883,7 @@ def update_credential(id): # Try to save to the archive try: Credential( - id=f'{id}-{revision}', + id='{0}-{1}'.format(id, revision), name=update['name'], data_type='archive-credential', credential_pairs=update['credential_pairs'], @@ -970,8 +925,8 @@ def update_credential(id): if services: service_names = [x.id for x in services] - msg = f'Updated credential "{cred.name}"' - msg += f'({cred.id}); Revision {cred.revision}' + msg = 'Updated credential "{0}" ({1}); Revision {2}' + msg = msg.format(cred.name, cred.id, cred.revision) graphite.send_event(service_names, msg) webhook.send_event('credential_update', service_names, [cred.id]) permissions = { diff --git a/tests/unit/confidant/routes/credentials_test.py b/tests/unit/confidant/routes/credentials_test.py index ed881726..80f97e80 100644 --- a/tests/unit/confidant/routes/credentials_test.py +++ b/tests/unit/confidant/routes/credentials_test.py @@ -501,11 +501,6 @@ def test_update_credential(mocker: MockerFixture, credential: Credential): 'confidant.routes.credentials.Credential.get', return_value=credential, ) - mocker.patch( - ('confidant.routes.credentials.Credential' - '.data_type_date_index.query'), - return_value=[], - ) mocker.patch( ('confidant.routes.credentials.credentialmanager' '.get_latest_credential_revision'), @@ -572,30 +567,7 @@ def test_update_credential(mocker: MockerFixture, credential: Credential): assert ret.status_code == 400 assert 'Credential Pairs cannot be empty.' == json_data['error'] - # Credential name already exists (ie: query returns a value) - mocker.patch( - 'confidant.routes.credentials.Credential.data_type_date_index.query', - return_value=[credential], - ) - ret = app.test_client().put( - '/v1/credentials/123', - headers={"Content-Type": 'application/json'}, - data=json.dumps({ - 'name': 'me', - 'documentation': 'doc', - 'credential_pairs': {'key': 'value'}, - }), - ) - json_data = json.loads(ret.data) - assert ret.status_code == 409 - assert 'Name already exists' in json_data['error'] - # All good - mocker.patch( - ('confidant.routes.credentials.Credential' - '.data_type_date_index.query'), - return_value=[], - ) mocker.patch( ('confidant.routes.credentials.servicemanager' '.pair_key_conflicts_for_services'),