fix: pre-request script variable hostname certificate resolution [INS-4733] #8249
Conversation
ryan-willis
force-pushed
the
fix/client-certificate-usage-in-requests
branch
from
e5a8312 to
67e40d0
Compare
| @@ -167,16 +168,31 @@ export async function initInsomniaObject( | |||
| localVars: localVariables, | |||
| }); | |||
|
|
|||
| const existClientCert = rawObj.clientCertificates != null && rawObj.clientCertificates.length > 0; | |||
| const certificate = existClientCert && rawObj.clientCertificates[0] ? | |||
| const reqUrl = toUrlObject(rawObj.request.url); | |||
There was a problem hiding this comment.
This makes sense to me to move this part from below, pls notice that the url parsing is not reliable for all cases as there could be templates in it (there's one card for it). such as representing both host and path with tag {{_.srvr}}{{ _.path }}, then this url will not be rendered as expected.
There was a problem hiding this comment.
Good point. I tried to avoid running the url through a full render but it looks like we might need to do that to cover this case
There was a problem hiding this comment.
Now that I'm looking at this again, I could perform the template render hack prior to calling getHost, what do you think?
There was a problem hiding this comment.
We also have a plan to improve the replaceIn method, let's optimize it later.
| { | ||
| disabled: rawObj.clientCertificates[0].disabled, | ||
| disabled: existingClientCert.disabled, |
There was a problem hiding this comment.
You might notice that there seems no perfect way to transform it between 2 sides, I'm thinking if we could just leave the certificate undefined at the beginning, if user specified the cert in the script, we prepend to the cert list, or return the original certs, pls let me know what you think.
There was a problem hiding this comment.
I'd much rather return original certs, but I'm thinking we revamp this later
There was a problem hiding this comment.
Yes we can revamp it later and the prepend operation is for those cases who would like to update the cert through script.
ryan-willis
force-pushed
the
fix/client-certificate-usage-in-requests
branch
from
1db77f7 to
9ba2476
Compare
This fixes two distinct, but closely related issues:
When using a variable reference in the request URL for the hostname and a pre-request script with non-empty contents, client certs would be filtered using the un-rendered template text before sending the request and yield an error (part 1 of the new smoke test).
When gathering context to send a request that has a pre-request script with non-empty contents, the first cert in the list was always selected, disregarding the disabled flag and the hostname on the cert (part 2 of the new smoke test).