-
Notifications
You must be signed in to change notification settings - Fork 1
feat(jose): add entropy verification for weak secrets #85
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds runtime secret entropy and byte-length validation: new error code Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/core/src/jose.ts`:
- Around line 26-28: Wrap the call to createSecret(salt) in a try/catch so any
InvalidSecretError thrown by the jose library is caught and rethrown as the core
error code INVALID_SALT_SECRET_VALUE (preserving the original error
message/stack as inner/cause). Specifically, surround the existing
createSecret(salt) invocation with error handling that detects
jose.InvalidSecretError (or any thrown error) and throws a new core error (use
your project's core error class/constructor and code INVALID_SALT_SECRET_VALUE)
so salt validation failures are reported through the core taxonomy; keep
createDerivedSalt(secret) and the salt assignment unchanged.
In `@packages/jose/CHANGELOG.md`:
- Line 13: Update the changelog entry text to accurately reflect createSecret's
validation: change "32 characters" to "32 bytes", correct "6 bits" to "4 bits"
of entropy per character, and fix the typo "signin" to "signing"; reference the
createSecret function name in the entry so readers know the behavior is enforced
by that function.
In `@packages/jose/src/secret.ts`:
- Around line 29-38: The validation mismatches: change the byteLength check and
entropy threshold/messages to be consistent—use secret.length (character count)
instead of new TextEncoder().encode(secret).byteLength when enforcing the "at
least 32 characters" rule, update the entropy threshold check on
getEntropy(secret) to compare against 6 (not 4) if getEntropy returns bits per
character, and revise the thrown InvalidSecretError messages to accurately
reference "characters" and "6 bits per character" for the checks in the secret
validation block (functions/identifiers: secret, TextEncoder usage, getEntropy,
InvalidSecretError).
🧹 Nitpick comments (1)
packages/jose/test/index.test.ts (1)
213-216: Rename test to reflect entropy failure.
The test name says “with at least 32 bytes,” but it now expects a low-entropy rejection. Renaming avoids confusion when scanning failures.✏️ Suggested rename
- test("createSecret with string secret with at least 32 bytes", () => { + test("createSecret rejects low-entropy string secret (>= 32 bytes)", () => { const secretString = "this-is-a-very-secure-and-long-secret" expect(() => createSecret(secretString)).toThrow("Secret string must have an entropy of at least 6 bits per character") })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jose/src/secret.ts (1)
30-43:⚠️ Potential issue | 🟠 MajorValidate byte length for non-string secrets too.
Uint8Arraysecrets bypass the minimum-length enforcement, so very short binary secrets pass validation. That undermines the new security guarantees.🔒 Proposed fix
export const createSecret = (secret: SecretInput, length: number = 32) => { if (!Boolean(secret)) throw new InvalidSecretError("Secret is required") if (typeof secret === "string") { const byteLength = new TextEncoder().encode(secret).byteLength if (byteLength < length) { throw new InvalidSecretError(`Secret string must be at least ${length} bytes long`) } const entropy = getEntropy(secret) if (entropy < MIN_SECRET_ENTROPY_BITS) { throw new InvalidSecretError("Secret string must have an entropy of at least 6 bits per character") } return new Uint8Array(Buffer.from(secret, "utf-8")) } - return secret + if (secret instanceof Uint8Array && secret.byteLength < length) { + throw new InvalidSecretError(`Secret must be at least ${length} bytes long`) + } + return secret }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/jose/test/index.test.ts`:
- Around line 213-216: The test expectation and the thrown error string are
inconsistent with the MIN_SECRET_ENTROPY_BITS constant (set to 4); update the
error text in the createSecret implementation (packages/jose/src/secret.ts -
function createSecret) to say "Secret string must have an entropy of at least 4
bits per character" and then update the test (packages/jose/test/index.test.ts -
the test "createSecret with string secret with at least 32 bytes") to expect the
same exact message so both the thrown error and the assertion match the
MIN_SECRET_ENTROPY_BITS value.
🧹 Nitpick comments (1)
packages/jose/test/index.test.ts (1)
207-237: Redundant undefined-secret coverage—consider consolidating.
The new undefined test duplicates the earlier “createSecret without secret” case. Optional: merge null/undefined cases into a single table-driven test for concision.♻️ Consolidation idea
- test("createSecret without secret", () => { - const secret = undefined - expect(() => createSecret(secret as unknown as string)).toThrow("Secret is required") - }) ... - test("createSecret with null secret", () => { - const secret = null - expect(() => createSecret(secret as unknown as string)).toThrow("Secret is required") - }) - - test("createSecret with undefined secret", () => { - const secret = undefined - expect(() => createSecret(secret as unknown as string)).toThrow("Secret is required") - }) + test.each([ + ["null", null], + ["undefined", undefined], + ])("createSecret with %s secret", (_label, secret) => { + expect(() => createSecret(secret as unknown as string)).toThrow("Secret is required") + })
Description
This pull request adds entropy verification for weak secrets via the
createSecretfunction, which is used internally bycreateJWT,createJWE,createJWS, andcreateDeriveKeys. The validation ensures that secrets meet minimum security requirements: at least 32 bits of length and a minimum of 4 bits of entropy per character, helping prevent the use of weak or guessable secrets.Additionally, all utilities defined in
secret.tsare now re-exported from the entry module of the@aura-stack/josepackage.Following the same approach, the
createSecretfunction was also implemented in the core module of@aura-stack/authto validate the salt used during key derivation, ensuring it meets the same strength and entropy requirements.Summary by CodeRabbit
New Features
Improvements
Documentation