Replace JWT dependency and remove vulnerable crypto chain#992
Replace JWT dependency and remove vulnerable crypto chain#992PeterDaveHello wants to merge 1 commit into
Conversation
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. Keep the existing JWT claims, custom runtime header, token cache behavior, and AMO authorization format intact. Cache entries now track the API key so key changes regenerate tokens instead of reusing a still-valid token signed for another key. Update uuid and pre-commit to fixed versions without broad force upgrades. Add tests that verify HS256 signatures with node:crypto and cover runtime cache reuse, key changes, expiration, invalid keys, and AMO signing.
📝 WalkthroughWalkthroughReplaces the jsonwebtoken library with a custom HS256 JWT signing utility built on ChangesJWT signing migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Script as submit-stores/jwt-token-generator
participant Signer as signHs256Jwt
participant Hashes as "`@noble/hashes`"
Script->>Signer: signHs256Jwt(payload, secret, header)
Signer->>Signer: base64url encode header and payload
Signer->>Hashes: hmac(sha256, secret, signingInput)
Hashes-->>Signer: signature bytes
Signer-->>Script: header.payload.signature
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}
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.
| function signHs256Jwt(payload, secret, header = { alg: 'HS256', typ: 'JWT' }) { | ||
| const encodedHeader = encodeJson(header) |
There was a problem hiding this comment.
The secret parameter is passed directly to textEncoder.encode(secret). If secret is undefined (for example, due to a missing or malformed API key), TextEncoder will encode the string "undefined" as the secret. This leads to a severe security vulnerability where tokens are signed with a weak, publicly known static string. We should explicitly validate that secret is a non-empty string before encoding it.
| function signHs256Jwt(payload, secret, header = { alg: 'HS256', typ: 'JWT' }) { | |
| const encodedHeader = encodeJson(header) | |
| function signHs256Jwt(payload, secret, header = { alg: 'HS256', typ: 'JWT' }) { | |
| if (typeof secret !== 'string' || !secret) { | |
| throw new TypeError('Secret must be a non-empty string') | |
| } | |
| const encodedHeader = encodeJson(header) |
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.
| function signHs256Jwt(payload, secret, header = { alg: 'HS256', typ: 'JWT' }) { | ||
| const encodedHeader = encodeJson(header) | ||
| const encodedPayload = encodeJson(payload) | ||
| const signingInput = `${encodedHeader}.${encodedPayload}` |
| function decodeTokenPart(part) { | ||
| return JSON.parse(Buffer.from(part, 'base64url').toString('utf8')) | ||
| } |
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| function signHs256Jwt(payload, secret, header = { alg: 'HS256', typ: 'JWT' }) { | ||
| const encodedHeader = encodeJson(header) | ||
| const encodedPayload = encodeJson(payload) | ||
| const signingInput = `${encodedHeader}.${encodedPayload}` | ||
| const signature = hmac(sha256, textEncoder.encode(secret), textEncoder.encode(signingInput)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject invalid secrets and pin the header to HS256.
Line 20 lets callers advertise any alg, but Line 23 always signs with HMAC-SHA256, so this helper can generate self-inconsistent JWTs. Also, TextEncoder.encode(secret) string-coerces undefined/null, which means a missing secret now signs with 'undefined' instead of failing fast. Since this is the new shared signer for runtime tokens and AMO auth, both cases turn configuration mistakes into hard-to-diagnose auth failures.
Proposed fix
-function signHs256Jwt(payload, secret, header = { alg: 'HS256', typ: 'JWT' }) {
- const encodedHeader = encodeJson(header)
+function signHs256Jwt(payload, secret, header = {}) {
+ if (typeof secret !== 'string' || secret.length === 0) {
+ throw new TypeError('JWT secret must be a non-empty string')
+ }
+ if (header.alg && header.alg !== 'HS256') {
+ throw new Error('signHs256Jwt only supports HS256')
+ }
+
+ const encodedHeader = encodeJson({ ...header, alg: 'HS256', typ: 'JWT' })
const encodedPayload = encodeJson(payload)
const signingInput = `${encodedHeader}.${encodedPayload}`
const signature = hmac(sha256, textEncoder.encode(secret), textEncoder.encode(signingInput))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function signHs256Jwt(payload, secret, header = { alg: 'HS256', typ: 'JWT' }) { | |
| const encodedHeader = encodeJson(header) | |
| const encodedPayload = encodeJson(payload) | |
| const signingInput = `${encodedHeader}.${encodedPayload}` | |
| const signature = hmac(sha256, textEncoder.encode(secret), textEncoder.encode(signingInput)) | |
| function signHs256Jwt(payload, secret, header = {}) { | |
| if (typeof secret !== 'string' || secret.length === 0) { | |
| throw new TypeError('JWT secret must be a non-empty string') | |
| } | |
| if (header.alg && header.alg !== 'HS256') { | |
| throw new Error('signHs256Jwt only supports HS256') | |
| } | |
| const encodedHeader = encodeJson({ ...header, alg: 'HS256', typ: 'JWT' }) | |
| const encodedPayload = encodeJson(payload) | |
| const signingInput = `${encodedHeader}.${encodedPayload}` | |
| const signature = hmac(sha256, textEncoder.encode(secret), textEncoder.encode(signingInput)) |
🤖 Prompt for 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.
In `@src/utils/hs256-jwt.mjs` around lines 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.
| exp: currentSeconds + timeoutSeconds, | ||
| iat: currentSeconds, | ||
| timestamp: currentSeconds, | ||
| } | ||
|
|
||
| jwtToken = jwt.sign(payload, secret, { | ||
| header: { | ||
| alg: 'HS256', | ||
| typ: 'JWT', | ||
| sign_type: 'SIGN', | ||
| }, | ||
| jwtToken = signHs256Jwt(payload, secret, { | ||
| alg: 'HS256', | ||
| typ: 'JWT', | ||
| sign_type: 'SIGN', | ||
| }) | ||
| tokenApiKey = apiKey | ||
| tokenExpiration = ms + timeoutSeconds * 1000 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Align the cache cutoff with the JWT exp.
Line 29 derives tokenExpiration from the original millisecond clock, while Line 18 rounds exp down to whole seconds. That leaves a 0-999 ms window where getToken() can reuse a token whose JWT is already expired, which can show up as intermittent 401s at the cache boundary.
Proposed fix
const payload = {
api_key: id,
exp: currentSeconds + timeoutSeconds,
iat: currentSeconds,
timestamp: currentSeconds,
}
jwtToken = signHs256Jwt(payload, secret, {
alg: 'HS256',
typ: 'JWT',
sign_type: 'SIGN',
})
tokenApiKey = apiKey
- tokenExpiration = ms + timeoutSeconds * 1000
+ tokenExpiration = payload.exp * 1000
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exp: currentSeconds + timeoutSeconds, | |
| iat: currentSeconds, | |
| timestamp: currentSeconds, | |
| } | |
| jwtToken = jwt.sign(payload, secret, { | |
| header: { | |
| alg: 'HS256', | |
| typ: 'JWT', | |
| sign_type: 'SIGN', | |
| }, | |
| jwtToken = signHs256Jwt(payload, secret, { | |
| alg: 'HS256', | |
| typ: 'JWT', | |
| sign_type: 'SIGN', | |
| }) | |
| tokenApiKey = apiKey | |
| tokenExpiration = ms + timeoutSeconds * 1000 | |
| exp: currentSeconds + timeoutSeconds, | |
| iat: currentSeconds, | |
| timestamp: currentSeconds, | |
| } | |
| jwtToken = signHs256Jwt(payload, secret, { | |
| alg: 'HS256', | |
| typ: 'JWT', | |
| sign_type: 'SIGN', | |
| }) | |
| tokenApiKey = apiKey | |
| tokenExpiration = payload.exp * 1000 |
🤖 Prompt for 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.
In `@src/utils/jwt-token-generator.mjs` around lines 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.
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.
Keep the existing JWT claims, custom runtime header, token cache behavior, and AMO authorization format intact. Cache entries now track the API key so key changes regenerate tokens instead of reusing a still-valid token signed for another key.
Update uuid and pre-commit to fixed versions without broad force upgrades. Add tests that verify HS256 signatures with node:crypto and cover runtime cache reuse, key changes, expiration, invalid keys, and AMO signing.
Summary by CodeRabbit
New Features
Bug Fixes
Tests