Skip to content

handle RSA_R_KEY_SIZE_TOO_SMALL error#6309

Merged
anonrig merged 1 commit intomainfrom
yagiz/fix-crypto-error
Mar 12, 2026
Merged

handle RSA_R_KEY_SIZE_TOO_SMALL error#6309
anonrig merged 1 commit intomainfrom
yagiz/fix-crypto-error

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Mar 12, 2026

No description provided.

@anonrig anonrig requested a review from jasnell March 12, 2026 15:31
@anonrig anonrig requested review from a team as code owners March 12, 2026 15:31
@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 12, 2026

I've now reviewed the full context of both changed files. Let me analyze:

Changes summary:

  1. impl.c++: Adds RSA_R_KEY_SIZE_TOO_SMALL error mapping in two places — throwOpensslError() (via MAP_ERROR macro) and consumeAllOpensslErrors() (via direct case statement). Both map to "RSA key size is too small.".
  2. impl-test.c++: Adds a test verifying the new error is converted to a DOMException(OperationError) instead of an internal error.

Review analysis:

  • The change converts what was previously an internal/opaque BoringSSL error into a user-facing DOMOperationError. This is a behavior change (the error message and exception type change), but it's strictly an improvement: the error was already thrown, just with an unhelpful internal message. Converting it to a clearer user-facing message does not break backward compatibility since no reasonable code would depend on the specific internal OpenSSL error string format.
  • Both code paths (throwOpensslError and consumeAllOpensslErrors) are updated consistently with the same error string.
  • The test is well-structured, following the existing test pattern exactly.
  • No security concerns — this only changes error message text, not control flow.
  • No compat flag needed — this is an error path improvement that makes errors more user-friendly and reduces Sentry noise.
  • No memory safety issues.

This is a clean, minimal, well-tested change. No actionable issues.

LGTM

github run

@anonrig anonrig enabled auto-merge March 12, 2026 15:33
@anonrig anonrig merged commit 41fd434 into main Mar 12, 2026
41 of 43 checks passed
@anonrig anonrig deleted the yagiz/fix-crypto-error branch March 12, 2026 16:44
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.

2 participants