feat(connect-form): Allow saving invalid but well-formed strings #2861
Conversation
mcasimir
changed the title
| // if a connection has been saved already we only want to update the lastUsed | ||
| // attribute, otherwise we are going to save the entire connection info. | ||
| const connectionInfoToBeSaved = | ||
| (await connectionStorage.load(connectionInfo.id)) ?? connectionInfo; |
There was a problem hiding this comment.
Do we need to reload the entire connection here? Would it make sense to check for lastUsed instead?
There was a problem hiding this comment.
we need to reload for the update, but we could do a check first, something like this?
const connectionInfoToBeSaved = connectionInfo.lastUsed && (await connectionStorage.load(connectionInfo.id)) ?? connectionInfo;There was a problem hiding this comment.
Ah gotcha we are reloading here to pull in the original state of the connection then so the changes that are currently there do not override. This is probably a nicer way to do it than keeping around a clone of the original state of the connection then, I think both have implications if a user is doing things in another window.
There was a problem hiding this comment.
We are only replicating the old behavior here, i was surprised this is how the old connection model worked.
There was a problem hiding this comment.
I think the suggestion I made for using lastUsed might not actually do anything then? Sorry!
There was a problem hiding this comment.
ah true, actually it would cause the entire thing to be saved on connect the first time, even if it was saved before, but just never used
There was a problem hiding this comment.
Changed it back. Let's wait for @addaleax before merging, It would be great to have some feedback cause she wasn't fond of relaxing the validation too much. We can easily drop the looseValidation from ensureWellFormedConnectionString in case
mcasimir
changed the title
| warnings.push({ | ||
| message: | ||
| 'Disabling certificate validation is not recommended as it may create a security vulnerability', | ||
| message: `TLS/SSL certificate validation is disabled. For a more secure connection enable certificate validation if possible.`, |
There was a problem hiding this comment.
I did like that this explicitly used the phrase “vulnerability”. Usually, that’s what it is, and “more secure connection” makes it sound like the connection still gives some security guarantees, which it effectively does not.
It’s nice to use positive language, but we should really push users towards never using these options.
There was a problem hiding this comment.
@mmarcon what's your opinion here? For this PR I can just keep the old copy and maybe do another one.
There was a problem hiding this comment.
What if we said something like:
TLS/SSL certificate validation is disabled. If possible, enable certificate validation to avoid security vulnerabilities.
There was a problem hiding this comment.
Going to merge this one and open another PR to discuss the copy for TLS
There was a problem hiding this comment.
I think that’s fine, yeah.
(Think about how browsers do this – in both Chrome and Firefox you’ll need to go to an Advanced section and are only then presented with the option to actually go to the site. And here, this potentially affects our customer’s production databases, where the risk probably tends to be higher-impact than a person visiting a bad-HTTPS site.)
| warnings.push({ | ||
| message: | ||
| 'Connecting without tls is not recommended as it may create a security vulnerability.', | ||
| 'Connecting to a remote server without TLS/SSL is not recommended. For a more secure connection enable TLS/SSL if possible.', |
mcasimir
changed the title
4 participants
This came up from a chat with @mmarcon and requests by customers on slack. The gist of the change is:
pineapple, but users can save connections that would not work with the driver, like those missing passwords.lastUsedfield instead of saving all the changes. NOTE: this replicates the behavior of the old connection form.