Sanitize the string to avoid a connection string injection #532
Conversation
| 'port': param_store.get_parameter(env=envname, path='aurora/port'), | ||
| 'db': param_store.get_parameter(env=envname, path='aurora/db'), | ||
| 'host': host, | ||
| 'db': database, |
There was a problem hiding this comment.
Hello @nikpodsh I see that you removed the port. Do we need it to make a connection or was it unnecessary?
There was a problem hiding this comment.
Yeah, I double checked and it turned out that the port was used. Thanks for bringing this up!
We don't use the port while creating the database connection in the backend. So it should always roll back to the default value 5432.
I can re-add it and this time make the port be configurable on the backend or delete the usages of the port (two in the local env and one quicksight#create_analysis ). What do you think would be better ?
There was a problem hiding this comment.
I don't think adding the port as a configuration adds any value for the user, just more "work" for them. When you say deleting the port you mean hardcoding it in local and quicksight?
There was a problem hiding this comment.
Yes, just hardcoding. By deleting I meant the deleting a configurable parameter (which is configurable only in a few places)
backend/dataall/db/dbconfig.py
Outdated
|
|
||
| @staticmethod | ||
| def _sanitize(regex, string: str) -> str: | ||
| return re.sub(regex, "", string) |
There was a problem hiding this comment.
rather than sanitizing and modifying the string, we should probably raise an exception if the string is invalid. modifying the inputs can have unexpected results for the user
There was a problem hiding this comment.
Thanks! I will raise an error if the string is not the same after sanitizing.
### Feature or Bugfix Release PR with the following list of features. Refer to each PR for the details ### Detail - #498 - #482 - #543 - #524 (which also solves #531) - #532 - #535 - #497 - #515 - #529 - #562 - #455 - #572 - #567 - #573 - #579 - #578 - #582 ### Breaking changes - release notes -⚠️ IMPORTANT: upgrade to a version >V1.5.0 before upgrading to V1.6 to avoid deletion of resources in custom resource deletion -⚠️ IMPORTANT: requires an update of environments and then datasets after upgrading. Either using cdk.json parameter `enable_update_dataall_stacks_in_cicd_pipeline`, waiting for overnight update stack task, or manually updating first environments and then datasets - CloudFront distribution replace for #529 - Additional EC2 permissions in CDK Synth CodeBuild stage for #543 --> this can be avoided by upgrading to v1.5.6 before upgrading to v1.6.0 - local development affected by more restrictive pivotRole trust policy ### Relates V1.6.0 release By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: Gezim Musliaj <102723839+gmuslia@users.noreply.github.com> Co-authored-by: Noah Paige <69586985+noah-paige@users.noreply.github.com> Co-authored-by: nikpodsh <124577300+nikpodsh@users.noreply.github.com> Co-authored-by: chamcca <40579012+chamcca@users.noreply.github.com> Co-authored-by: Nikita Podshivalov <nikpodsh@amazon.com> Co-authored-by: dbalintx <132444646+dbalintx@users.noreply.github.com> Co-authored-by: mourya-33 <134511711+mourya-33@users.noreply.github.com>
Feature or Bugfix
Feature
Detail
We should avoid possible connection injection. To do so, we have to sanitize and validate inputs for the database
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.