fix(NODE-3487): check for nullish aws mechanism property #2951
Conversation
| { | ||
| accessKeyId: username, | ||
| secretAccessKey: password, | ||
| token |
There was a problem hiding this comment.
This should've been sessionToken, I moved the whole object declaration above.
src/cmap/auth/mongodb_aws.ts
Outdated
| a: options.headers.Authorization, | ||
| d: options.headers['X-Amz-Date'] | ||
| }; | ||
| if (sessionToken != null && sessionToken !== '') { |
There was a problem hiding this comment.
Should this just stay false-y?
There was a problem hiding this comment.
Yeah I think falsey-ness works best here, anything except a non-empty string should probably be ignored.
There was a problem hiding this comment.
SGTM or I can also do typeof sessionToken === 'string' && sessionToken != ''
Explicit permitting of non-empty string typed variables would be the strictest
There was a problem hiding this comment.
if we only ever read the credentials in from the env variable, there is no chance of it being anything other than a string, so I think the if (sessionToken) check would be perfectly fine, and if we read it in from anywhere else, we should really be doing this validation at that entry point, not here
There was a problem hiding this comment.
indeed, and we do validate this in MongoCredentials so I put this back to a simple faleyness check
| 'X-Amz-Date': string; | ||
| }; | ||
| }; | ||
| } |
There was a problem hiding this comment.
I inlined our own definitions of the types to hold us strictly to our usage, also this catches the mistake of passing in token instead of sessionToken
src/cmap/auth/mongodb_aws.ts
Outdated
| a: options.headers.Authorization, | ||
| d: options.headers['X-Amz-Date'] | ||
| }; | ||
| if (sessionToken != null && sessionToken !== '') { |
There was a problem hiding this comment.
Yeah I think falsey-ness works best here, anything except a non-empty string should probably be ignored.
src/cmap/auth/mongodb_aws.ts
Outdated
|
|
||
| // If all three defined, include sessionToken, else include username and pass, else no credentials | ||
| const awsCredentials = | ||
| accessKeyId != null && secretAccessKey != null && sessionToken != null |
There was a problem hiding this comment.
so it's ok to have an empty string sessionToken in the options here but not in L146? what are the rules precisely?
There was a problem hiding this comment.
I think based on your reply to my other comment, the type validation should be done in the MongoCredentials class. Here we should rely on falseness for all 3 properties.
The empty string filtering below was meant to handle the case where its being set to an empty string by the user to explicitly not make use of the environment variable. But being empty we don't actually want to send it in the auth process. It probably would be harmless to send it.. maybe we're doing too much special casing.
Should I just follow the spec precisely?
Which would be: If the token is set by the user in the options you MUST use it, regardless of whether it is an empty string?
There was a problem hiding this comment.
Sorry I am still a bit lost here. My understanding is that we do want the user to be able to override the env variable by passing in any non-null value, that's not in question. But it was also my understanding that if sessionToken === '' then we don't want to sent it to aws, but by setting it here and passing it in L139, we are in fact passing the empty string, no? I was comparing L139 to L146 where we explicitly omit it in L146 but not in L139. What am I missing?
There was a problem hiding this comment.
You are not missing anything, I am, 😅 fixed the code here, I was getting mixed up in the assertions. We just confirm non-falsy-ness here since type validation has already been done in the MongoCredentials, and we want to skip using the sessionToken if it is an empty string.
src/cmap/auth/mongodb_aws.ts
Outdated
| a: options.headers.Authorization, | ||
| d: options.headers['X-Amz-Date'] | ||
| }; | ||
| if (sessionToken != null && sessionToken !== '') { |
There was a problem hiding this comment.
if we only ever read the credentials in from the env variable, there is no chance of it being anything other than a string, so I think the if (sessionToken) check would be perfectly fine, and if we read it in from anywhere else, we should really be doing this validation at that entry point, not here
nbbeeken
force-pushed
the
NODE-3487/aws-session-env
branch
from
d4b4d43
to
4501180
Compare
Checks for nullish aws mechanism property instead of falsy, but usage in the AWS mechanism propery checks for falsiness, this allows users to pass an empty string to avoid using the environment variable.
(needs 3.x port)