feat!: unify secret function signature across all code paths by paolochiodi · Pull Request #409 · fastify/fastify-jwt

paolochiodi · 2026-05-13T13:34:52Z

BREAKING CHANGE: The secret function signature is now called with (context, callback) where context is { operation, payload, header?, signature?, request? } instead of the previous inconsistent signatures (request, token, callback) or (request, payload).

This PR fixes #388, by changing how secret is handled - especially when it's a function. This happens in three ways:

resolve secret before calling fast-jwt (specific fix for Secret function is called with different signature depending on context #388)
uniform signature for the secret function across request and instance level functions: I propose to just pass a context object as first parameter, which will include the payload (always present) and an optional request (when called from request level methods and a request context is present)
uniform signature for the secret function when called from either sign or verify - making the difference explicit by adding an operation attribute to the context. The other difference is that when called from a sign function, the context will be missing header and signature

Tests have been added to cover function being called on instances, and checks for the various context options.

_Disclaimer: Claude Opus was used to assist development

Checklist

run npm run test && npm run benchmark --if-present
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

Copilot

Pull request overview

This PR implements the breaking change described in #388 by standardizing how secret functions are invoked across all signing/verifying entry points. It introduces a single (context, callback) secret-function signature (with a context.operation discriminator) and resolves function-based secrets before calling fast-jwt, plus updates docs, typings, and tests accordingly.

Changes:

Introduce a SecretContext object (operation, payload, and for verify also header/signature, plus optional request) and update TypeScript types to match.
Update runtime logic to resolve function-based secrets before creating/verifying tokens (including adding a complete decoder for verify context).
Expand tests and README documentation to cover the new signature and behavior (including decode error cases).

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`jwt.js`	Adds `resolveSecret`, builds `SecretContext`, and changes sign/verify flows to resolve secrets per call when needed.
`types/jwt.d.ts`	Defines `SecretContext*` types and updates `Secret` function signatures to `(context, cb)` / `(context) => Promise`.
`types/jwt.tst.ts`	Updates type tests to use the new secret function signatures.
`test/jwt.test.js`	Adds coverage for server-method sign/verify with function secrets and context shape; adds decode error tests.
`README.md`	Documents the new secret function context/callback signature and callback requirement for server methods when using function secrets.

Comments suppressed due to low confidence (3)

jwt.js:336

In sign(), when a callback is provided you always resolve secretOrPrivateKey and then overwrite signerConfig.options.key via withResolvedKey(...). This means a per-call options.key override is ignored in callback mode, even though the README documents sign.key overriding the plugin secret. The callback code path should prefer an explicitly provided signerConfig.options.key and only call the plugin secret function when no key override is present.

    const cb = signerConfig.callback
    const context = { operation: 'sign', payload }
    resolveSecret(secretOrPrivateKey, context, function (err, secret) {
      if (err) return cb(err)
      const resolvedOptions = withResolvedKey(signerConfig.options, secret)
      const localSigner = createSigner(resolvedOptions)
      cb(null, localSigner(payload))
    })

jwt.js:363

In verify(), the callback code path always decodes the token and resolves secretOrPublicKey, then overwrites verifierConfig.options.key with that value. This ignores a per-call options.key override in callback mode (contrary to the documented verify.key behavior). It should only call the plugin-level secret function when verifierConfig.options.key is not already provided.

    const cb = verifierConfig.callback
    const decoded = completeDecoder(token)
    const context = { operation: 'verify', header: decoded.header, payload: decoded.payload, signature: decoded.signature }
    resolveSecret(secretOrPublicKey, context, function (err, secret) {
      if (err) return cb(err)
      const resolvedOptions = withResolvedKey(verifierConfig.options, secret)
      const localVerifier = getVerifier(resolvedOptions)
      cb(null, localVerifier(token))
    })

jwt.js:352

The assert(hasStaticPublicKey, 'callback is required when secret is a function') check blocks synchronous fastify.jwt.verify(token, { key: ... }) when the plugin secret is a function, even though a static per-call options.key should make verification possible without resolving the secret function. Consider basing this assertion on whether verifierConfig.options.key is set (and static) rather than the plugin secret type.

    if (typeof verifierConfig.callback !== 'function') {
      assert(hasStaticPublicKey, 'callback is required when secret is a function')
      let localVerifier = verifier
      if (options && typeof options !== 'function') {
        localVerifier = getVerifier(verifierConfig.options)
      }
      return localVerifier(token)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

paolochiodi · 2026-05-18T13:57:57Z

+  if (typeof secretValue !== 'function') {
+    return callback(null, secretValue)
+  }
+
+  const result = secretValue(context, callback)
+
+  if (result && typeof result.then === 'function') {
+    result.then(secret => callback(null, secret), callback)


Implemented a mitigation similar to the suggested change.

paolochiodi · 2026-05-14T14:15:46Z

+    if (typeof signerConfig.callback !== 'function') {
+      assert(hasStaticPrivateKey, 'callback is required when secret is a function')
+      let localSigner = signer
+      if (options && typeof options !== 'function') {
+        localSigner = createSigner(signerConfig.options)
+      }
      return localSigner(payload)


After investigation, the issue here seems more fundamental: sign options object allow the caller to override the key attribute, and the code doesn't account for that - later also using secretOrPrivateKey instead of options.key - potentially losing the override.

I probably missed this because of the naming discrepancy: what is called secret in the plugin setup options, is now called key in the sign function.

I'll consider a change.

I fixed the issue as in my previous comment. Now the code for the instance-level sign and verify properly use the options input to override the plugin config.
As part of that I also expanded options.key type to also optionally be a function (it was either a string or buffer beforehand).
This was necessary because even if the type only allowed string or buffer, not runtime check actually controlled that - the options were just passed to fast-jwt which supported functions (although with a different signature). This made plain javascript and typescript code behavior different.
Instead of adding runtime checks I expanded the type to match the actual code.

However, I wanted to highlight another discrepancy I decided to not address here. The same logical concept is called key in the sign and verify options, but secret on the plugin config.

paolochiodi · 2026-05-14T14:17:46Z

  function verify (token, options, callback) {
    assert(token, 'missing token')
    assert(secretOrPublicKey, 'missing secret')

-    let localVerifier = verifier
-
    const localOptions = convertTemporalProps(options, true)
    const verifierConfig = checkAndMergeVerifyOptions(localOptions, callback)

-    if (options && typeof options !== 'function') {
-      localVerifier = getVerifier(verifierConfig.options)
-    }
-
-    if (typeof verifierConfig.callback === 'function') {
-      const result = localVerifier(token)
-      verifierConfig.callback(null, result)
-    } else {
+    if (typeof verifierConfig.callback !== 'function') {
+      assert(hasStaticPublicKey, 'callback is required when secret is a function')
+      let localVerifier = verifier
+      if (options && typeof options !== 'function') {
+        localVerifier = getVerifier(verifierConfig.options)
+      }
      return localVerifier(token)
    }
+
+    const cb = verifierConfig.callback
+    const decoded = completeDecoder(token)
+    const context = { operation: 'verify', header: decoded.header, payload: decoded.payload, signature: decoded.signature }
+    resolveSecret(secretOrPublicKey, context, function (err, secret) {
+      if (err) return cb(err)
+      const resolvedOptions = withResolvedKey(verifierConfig.options, secret)
+      const localVerifier = getVerifier(resolvedOptions)
+      cb(null, localVerifier(token))
+    })


I guess this is a fair comment. I'll experiment with keeping a fast-path here, and see how much complexity is added to the code

Added back a fast-path when key is static and no custom option is provided to the methods.

mcollina · 2026-05-15T13:06:03Z

are you adressing those feedback?

paolochiodi · 2026-05-18T07:25:59Z

are you adressing those feedback?

Yes, working on it - it would take me a few days.

Fixes fastify#388 BREAKING CHANGE: The secret function signature is now called with (context, callback) where context is { operation, payload, header?, signature?, request? } instead of the previous inconsistent signatures (request, token, callback) or (request, payload).

The fixes in the previous commit broke the options override for key/secret. This commit brings them back in the new setup.

paolochiodi · 2026-05-18T14:06:30Z

I've addressed copilot's feedback, rebased after and resolved conflicts that came from #408 being merged.
I've responded to the threads with my take and explanations.

I'm going to trigger another review from copilot.

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

+    const decoded = completeDecoder(token)
+    const context = { operation: 'verify', header: decoded.header, payload: decoded.payload, signature: decoded.signature }
+    resolveSecret(verifierConfig.options.key, context, function (err, secret) {
+      if (err) return cb(err)
+      try {
+        const resolvedOptions = withResolvedKey(verifierConfig.options, secret)
+        const localVerifier = getVerifier(resolvedOptions)
+        cb(null, localVerifier(token))
+      } catch (error) {
+        cb(error)
+      }
+    })


+function resolveSecret (secretValue, context, callback) {
+  if (typeof secretValue !== 'function') {
+    return callback(null, secretValue)
+  }
+
+  let called = false
+  function once (err, val) {
+    if (called) return
+    called = true
+    callback(err, val)
+  }
+
+  const result = secretValue(context, once)
+
+  if (result && typeof result.then === 'function') {
+    result.then(secret => once(null, secret), once)
  }


-    const decodedToken = decode(token, options.decode || decodeOptions)
+    let completeDecode
+    try {
+      completeDecode = completeDecoder(token)


+    let completeDecode
+    try {
+      completeDecode = completeDecoder(token)
+    } catch (error) {
+      // Ignoring the else branch because it's not possible to test it,
+      // it's just a safeguard for future changes in the fast-jwt library
+      if (error.code === TokenError.codes.malformed) {
+        return next(new AuthorizationTokenInvalidError(error.message))
+      } else if (error.code === TokenError.codes.invalidType) {
+        return next(new AuthorizationTokenInvalidError(error.message))
+      } /* c8 ignore start */ else {
+        return next(error)
+      } /* c8 ignore stop */
+    }


paolochiodi force-pushed the 388-unify-secret-signature branch from f823724 to 24d3e54 Compare May 13, 2026 13:40

paolochiodi mentioned this pull request May 13, 2026

Secret function is called with different signature depending on context #388

Open

2 tasks

simoneb requested a review from Copilot May 13, 2026 13:59

Copilot started reviewing on behalf of simoneb May 13, 2026 14:00 View session

Copilot AI reviewed May 13, 2026

View reviewed changes

paolochiodi added 2 commits May 18, 2026 14:46

fix: allow options to be overridden on instance methods

51f6595

The fixes in the previous commit broke the options override for key/secret. This commit brings them back in the new setup.

paolochiodi force-pushed the 388-unify-secret-signature branch from 30e552e to 51f6595 Compare May 18, 2026 12:53

paolochiodi added 2 commits May 18, 2026 15:01

feat: ensure callback only called once

d0372ab

fix: re-add fast-path for static keys

61fce4e

paolochiodi requested a review from Copilot May 18, 2026 14:06

Copilot AI reviewed May 18, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat!: unify secret function signature across all code paths#409

feat!: unify secret function signature across all code paths#409
paolochiodi wants to merge 4 commits into
fastify:mainfrom
paolochiodi:388-unify-secret-signature

paolochiodi commented May 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

paolochiodi May 18, 2026

Uh oh!

paolochiodi May 14, 2026

Uh oh!

paolochiodi May 18, 2026

Uh oh!

paolochiodi May 14, 2026

Uh oh!

paolochiodi May 18, 2026

Uh oh!

mcollina commented May 15, 2026

Uh oh!

paolochiodi commented May 18, 2026

Uh oh!

paolochiodi commented May 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

paolochiodi commented May 13, 2026

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

paolochiodi May 18, 2026

Choose a reason for hiding this comment

Uh oh!

paolochiodi May 14, 2026

Choose a reason for hiding this comment

Uh oh!

paolochiodi May 18, 2026

Choose a reason for hiding this comment

Uh oh!

paolochiodi May 14, 2026

Choose a reason for hiding this comment

Uh oh!

paolochiodi May 18, 2026

Choose a reason for hiding this comment

Uh oh!

mcollina commented May 15, 2026

Uh oh!

paolochiodi commented May 18, 2026

Uh oh!

paolochiodi commented May 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants