Skip to content

Conversation

@cirospaciari
Copy link
Member

@cirospaciari cirospaciari commented Apr 9, 2025

What does this PR do?

Fix: #18700

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

Test

@robobun
Copy link
Collaborator

robobun commented Apr 9, 2025

Updated 1:41 PM PT - Apr 9th, 2025

@cirospaciari, your commit 162abbce40499d5000b3bfc0f7a5107c036ad025 passed in Build #14730! 🎉


🧪   try this PR locally:

bunx bun-pr 18905

@cirospaciari cirospaciari marked this pull request as ready for review April 9, 2025 17:45
Copy link
Member

@dylan-conway dylan-conway left a comment

Choose a reason for hiding this comment

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

We should change the plaintextLength check to:

JSValue plaintextLengthValue = optionsValue.get(globalObject, Identifier::fromString(vm, "plaintextLength"_s));
RETURN_IF_EXCEPTION(scope, JSValue::encode({}));

if (!plaintextLengthValue.isUndefinedOrNull()) {
    std::optional<int32_t> maybePlaintextLength = plaintextLengthValue.tryGetAsInt32();
    if (!maybePlaintextLength || *maybePlaintextLength < 0) {
        return ERR::INVALID_ARG_VALUE(scope, globalObject, "options.plaintextLength"_s, plaintextLengthValue);
    }

    plaintextLength = *maybePlaintextLength;
}

This is the same conversion in new Cipher() for options.authTagLength. It's closer to value >>> 0 !== value in js

@dylan-conway
Copy link
Member

Could also add a getUIntOption function that wraps this logic

@cirospaciari
Copy link
Member Author

tryGetAsInt32

I think any other function todo the same would be basically result in the same amount of LOC that using tryGetAsInt32 so I just update to tryGetAsInt32

@dylan-conway dylan-conway merged commit 4ccf5c0 into main Apr 9, 2025
70 checks passed
@dylan-conway dylan-conway deleted the ciro/fix-crypto branch April 9, 2025 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The "options.encoding" property must be of type string. Received undefined

4 participants