Replace JWT dependency and remove vulnerable crypto chain#992
Open
PeterDaveHello wants to merge 1 commit into
Open
Replace JWT dependency and remove vulnerable crypto chain#992PeterDaveHello wants to merge 1 commit into
PeterDaveHello wants to merge 1 commit into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughReplaces ChangesJWT signing migration
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request replaces the heavy jsonwebtoken and crypto-browserify dependencies with a lightweight, custom HS256 JWT signing utility (hs256-jwt.mjs) powered by @noble/hashes. It also updates the token generator to regenerate tokens when the API key changes, upgrades several dependencies (such as uuid and pre-commit), and adapts the unit tests to verify tokens without relying on jsonwebtoken. Feedback is provided regarding a security vulnerability in the custom signing utility where an undefined secret would be encoded as the literal string "undefined", resulting in a weak signature; adding a validation check to ensure secret is a non-empty string is recommended.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Contributor
PR Summary by QodoReplace jsonwebtoken with minimal HS256 signer and drop crypto-browserify
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
There was a problem hiding this comment.
Pull request overview
This PR replaces the jsonwebtoken + browser crypto polyfill dependency chain with a small, project-scoped HS256 JWT signer (backed by @noble/hashes) used for both runtime tokens and Firefox AMO authorization, while keeping existing token formats and improving cache behavior to account for API key changes.
Changes:
- Introduce
signHs256Jwt(HS256-only signing) and migrate runtime + AMO JWT generation offjsonwebtoken. - Update runtime token caching to regenerate when the API key changes (not just on expiry) and preserve
iatbehavior. - Remove
crypto-browserifybuild fallback and add/adjust unit tests to verify HS256 signatures and cache behavior.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/utils/jwt-token-generator.test.mjs | Replaces jsonwebtoken decode/verify with explicit HS256 verification + adds cache/key-change coverage. |
| tests/unit/utils/hs256-jwt.test.mjs | Adds focused unit tests for the new HS256 JWT signer utility. |
| tests/unit/release/submit-stores.test.mjs | Verifies AMO Authorization JWT structure/signature without jsonwebtoken. |
| src/utils/jwt-token-generator.mjs | Switches runtime token generation to the new signer and extends cache invalidation to include API key changes. |
| src/utils/hs256-jwt.mjs | New HS256 JWT signing helper built on @noble/hashes. |
| scripts/submit-stores.mjs | Migrates AMO JWT creation from jsonwebtoken to the new HS256 signer. |
| package.json | Drops jsonwebtoken/crypto-browserify, adds @noble/hashes, bumps uuid and pre-commit. |
| package-lock.json | Lockfile updates reflecting dependency removals/additions and version bumps. |
| build.mjs | Removes webpack crypto fallback to drop the browser crypto polyfill tree. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Code Review by Qodo
1.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/hs256-jwt.mjs`:
- Around line 19-23: The signHs256Jwt helper should reject missing/invalid
secrets and stop accepting caller-provided JWT algorithms. Update signHs256Jwt
to validate secret before TextEncoder.encode(secret) so undefined/null/empty
values fail fast, and pin the header used by encodeJson to HS256/typ JWT instead
of trusting the header argument. Keep the HMAC-SHA256 signing logic in sync with
the header in signHs256Jwt so runtime tokens and AMO auth always produce
self-consistent JWTs.
In `@src/utils/jwt-token-generator.mjs`:
- Around line 18-29: The cache expiration in jwt-token-generator’s token
creation logic is computed from the millisecond clock, while the JWT payload exp
in the same function is based on whole seconds, so align the cache cutoff to the
same rounded seconds boundary used for exp. Update the token creation flow in
jwt-token-generator.mjs so tokenExpiration is derived from the same
currentSeconds/exp basis as jwtToken generation, ensuring getToken() stops
reusing a token as soon as its JWT is considered expired.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08e8fd43-80d9-4db7-b50f-3f08c7d1303f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
build.mjspackage.jsonscripts/submit-stores.mjssrc/utils/hs256-jwt.mjssrc/utils/jwt-token-generator.mjstests/unit/release/submit-stores.test.mjstests/unit/utils/hs256-jwt.test.mjstests/unit/utils/jwt-token-generator.test.mjs
💤 Files with no reviewable changes (1)
- build.mjs
PeterDaveHello
force-pushed
the
secure-jwt-dependency-cleanup
branch
2 times, most recently
from
692555a to
be5a63f
Compare
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
|
Code review by qodo was updated up to the latest commit be5a63f |
PeterDaveHello
force-pushed
the
secure-jwt-dependency-cleanup
branch
from
be5a63f to
dbccdfe
Compare
Contributor
|
Code review by qodo was updated up to the latest commit dbccdfe |
Comment on lines
+51
to
+53
| return ( | ||
| !jwtToken || getApiKeyFingerprint(apiKey) !== tokenApiKeyFingerprint || ms >= tokenExpiration | ||
| ) |
PeterDaveHello
force-pushed
the
secure-jwt-dependency-cleanup
branch
from
dbccdfe to
2f2adca
Compare
Contributor
|
Code review by qodo was updated up to the latest commit 2f2adca |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/utils/jwt-token-generator.mjs:63
- The PR description mentions this HS256 runtime token generator being used for “runtime tokens”, but there are currently no non-test call sites importing/using
getToken(repo-wide search only shows this file plus unit tests). If this is intended to be used in production, please wire it up in the relevant runtime code; otherwise consider updating the PR description (or removing dead code/tests if it’s no longer needed).
function getToken(apiKey) {
if (shouldRegenerateToken(apiKey)) {
generateToken(apiKey, 86400) // Hard-coded to regenerate the token every 24 hours
}
return jwtToken
}
export { getToken }
2f2adca to
59d9ce3
Compare
Contributor
✅ Action performedReview finished.
|
Contributor
|
Code review by qodo was updated up to the latest commit 59d9ce3 |
Use a focused HS256 signer backed by @noble/hashes for runtime and Firefox AMO tokens. This keeps browser builds from carrying a Node crypto polyfill tree while limiting signing to the algorithm this project actually uses. Validate JWT secrets and API key parts before signing so invalid configuration fails explicitly instead of producing tokens from empty or coerced values. Pin JWT headers to HS256 while preserving required custom fields such as ChatGLM's sign_type. Align runtime token cache expiry with the JWT exp claim to avoid reusing cached tokens at the expiration boundary. Compare API keys through a SHA-256 fingerprint so cache invalidation does not retain raw secrets. Preserve required claims and AMO authorization behavior with tests. Add coverage for HS256 signatures, token cache behavior, invalid keys, secret-only API key changes, and AMO signing. Update uuid for dependency hygiene. Keep pre-commit on its existing 1.x hook behavior and pin its transitive cross-spawn dependency to the fixed 7.0.6 release through npm overrides.
PeterDaveHello
force-pushed
the
secure-jwt-dependency-cleanup
branch
from
59d9ce3 to
3816d6e
Compare
Member
Author
|
/agentic_review |
Contributor
|
Code review by qodo was updated up to the latest commit 3816d6e |
Comment on lines
+11
to
15
| function parseApiKey(apiKey) { | ||
| const parts = apiKey.split('.') | ||
| if (parts.length !== 2) { | ||
| throw new Error('Invalid API key') | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use a focused HS256 signer backed by @noble/hashes for runtime tokens and Firefox AMO metadata signing. This keeps signing limited to the algorithm this project uses instead of carrying a Node crypto polyfill tree into browser builds.
Preserve the actual runtime JWT claim set, including the implicit iat claim previously added by jsonwebtoken, the custom runtime header, and AMO authorization format. Cache entries now track a SHA-256 API-key fingerprint so key changes regenerate tokens without retaining the raw secret.
Update uuid for dependency hygiene. Keep pre-commit on its existing 1.x hook behavior and pin its transitive cross-spawn dependency to 7.0.6 with npm overrides. Add tests that verify HS256 signatures with node:crypto and cover runtime cache reuse, key changes, expiration, invalid keys, retry token uniqueness, and AMO signing.
Summary by CodeRabbit
cross-spawnversion for pre-commit.