Skip to content

[msal-extensions] Extensions #1 #1830

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

Merged
merged 28 commits into from
Jul 13, 2020
Merged

[msal-extensions] Extensions #1 #1830

merged 28 commits into from
Jul 13, 2020

Conversation

sangonzal
Copy link
Contributor

  • Adds package.json, README, tsconfig, and sets directory structure
  • Adds Windows DPAPI node addon

@coveralls
Copy link

coveralls commented Jun 25, 2020

Coverage Status

Coverage remained the same at 80.998% when pulling 4cb10f0 on extensions-1 into 0f352a0 on dev.

{
isolate->ThrowException(v8::Exception::TypeError(
CreateUtf8String(isolate, "Second argument, optionalEntropy, must be null or an ArrayBuffer")));
}

Choose a reason for hiding this comment

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

I'd recommend constructing the correct C++ types ASAP after verifying the input types, that way you can work directly in C++, without having to think about v8 variadic types.

if (!info[1]->IsNull())
{
entropyBlob.pbData = reinterpret_cast<BYTE*>(node::Buffer::Data(info[1]));
entropyBlob.cbData = node::Buffer::Length(info[1]);

Choose a reason for hiding this comment

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

Do you have a guarantee that both of these buffers are null-terminated?
Or, does node::Buffer::Length have some other form of safety check to prevent an overrun?

Copy link
Member

@sameerag sameerag Jul 8, 2020

Choose a reason for hiding this comment

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

Edit: The data here (info[1]) is the user data that we are passing in to the windows API CryptProtectData(). dataIn and dataOut are pointers with specific length as specified in CryptProtectData usage of the DATA_BLOB.

Since this accounts for the data buffer and length, null-termination isn't needed. There will not be any overrun.

@sameerag sameerag mentioned this pull request Jun 26, 2020
1 task
Santiago Gonzalez and others added 7 commits June 29, 2020 09:37
Copy link
Member

@sameerag sameerag left a comment

Choose a reason for hiding this comment

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

Approving.

Comment on lines +48 to +53
v8::String::Utf8Value strData(isolate, info[2]);
std::string scope(*strData);
if (stricmp(scope.c_str(), "LocalMachine") == 0)
{
flags = CRYPTPROTECT_LOCAL_MACHINE;
}

Choose a reason for hiding this comment

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

Why are you passing a string parameter if all you want is a DWORD?

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 Node addon needs to be loaded on Mac and Linux as well, not just Windows, so we have use to platform agnostic parameters (my understanding is that DWORD is a type only defined for Windows)

@sameerag sameerag merged commit c6db556 into dev Jul 13, 2020
@sangonzal sangonzal added the msal-node-extensions Related to msal-node-extensions package label May 13, 2021
@tnorling tnorling deleted the extensions-1 branch October 6, 2021 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-node-extensions Related to msal-node-extensions package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants