Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Further Sanitize User Input #425

Merged
merged 8 commits into from
Apr 23, 2024
Merged

Conversation

alejandroroiz
Copy link
Contributor

@alejandroroiz alejandroroiz commented Apr 17, 2024

In this PR we fix the following issues:

  • Use f-strings in the create/update credential routes
  • Escape metadata in credential creation
  • Check if the credential name was updated
    • If it was then verify it does not conflict with an existing credential name
    • Also sanitize the new credential name
  • Check if new metadata was added and sanitize it
  • Check if new credential pairs were added and sanitize them
  • Escape documentation field in update & create

@alejandroroiz alejandroroiz changed the title fix bug bounty 659 Further Sanitize User Input Apr 17, 2024
@anshumanbh
Copy link

Also, cc: @diegonavarro-lyft @0xp4blo

Comment on lines +826 to +833
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the latency here? i remember you mentioned a high latency for check dup

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole dup check adds more latency since we need to decrypt credential pairs to verify if the cred is a full duplicate. this check is only for dup name, which is not as expensive

@alejandroroiz alejandroroiz merged commit 0bdd433 into master Apr 23, 2024
7 checks passed
@alejandroroiz alejandroroiz deleted the secfound-3585_alejandroroiz branch April 23, 2024 18:37
alejandroroiz added a commit that referenced this pull request Jul 24, 2024
alejandroroiz added a commit that referenced this pull request Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants