Skip to content

[jsonwebtoken]: Add standard fields #53435

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.

Sign up for GitHub

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

Merged
typescript-bot merged 10 commits into from
Jun 18, 2021
Merged

[jsonwebtoken]: Add standard fields #53435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
26b1f67
[jsonwebtoken]: Add standard fields
nandi95 May 27, 2021
3eb3183
[jsonwebtoken]: Add missing optional operator
nandi95 May 30, 2021
2942d3c
[jsonwebtoken]: Fixed errors
nandi95 Jun 4, 2021
ddee82f
[jsonwebtoken]: Added missing optional keys
nandi95 Jun 4, 2021
7196808
[jsonwebtoken]: Fixed test
nandi95 Jun 16, 2021
eeba270
[jsonwebtoken]: Exclude crit from the crit field
nandi95 Jun 16, 2021
9c619fc
[jsonwebtoken]: updated to the correct field
nandi95 Jun 16, 2021
e2ab849
[jsonwebtoken]: Added correct verify callback argument
nandi95 Jun 16, 2021
d5640a0
[jsonwebtoken]: Added return value to sync verify
nandi95 Jun 16, 2021
85afc23
[jsonwebtoken]: Simplified $ExpectType
nandi95 Jun 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 40 additions & 8 deletions types/jsonwebtoken/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
// Linus Unnebäck <https://github.com/LinusU>
// Ivan Sieder <https://github.com/ivansieder>
// Piotr Błażejewicz <https://github.com/peterblazejewicz>
// Nandor Kraszlan <https://github.com/nandi95>
// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped

/// <reference types="node" />
Expand Down Expand Up @@ -62,7 +63,7 @@ export interface SignOptions {
jwtid?: string;
mutatePayload?: boolean;
noTimestamp?: boolean;
header?: object;
header?: JwtHeader;
Copy link
Copy Markdown

@randonia randonia Aug 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this PR is months old now and has been approved and merged and published, but I just ran into a code breakage while upgrading @types/jsonwebtoken from 8.5.1->8.5.5 which should be compatible.

Given that: JwtHeader requires alg to be defined

and that: SignOptions specifies an algorithm?: Algorithm | undefined;

and that: jsonwebtoken's implementation of generating the header is:

  var header = Object.assign({
    alg: options.algorithm || 'HS256',
    typ: isObjectPayload ? 'JWT' : undefined,
    kid: options.keyid
  }, options.header);

This leads to the SignOptions.algorithm being completely ignored in 100% of usage.

Separately, the Types have been broken in the interface for SignOptions.

Example code that used to work in 8.5.1 that broke in 8.5.2:

JWT.sign(payload, privateKey, {
    algorithm: 'RS256',
    header: {
      kid,
    },
  });

^ the above now fails type checking as alg is not defined in the header.

The fix is pretty easy, move algorithm into header.alg, but more so I just wanted to point out that these changes were not semantic version compatible and now SignOptions.algorithm is completely ignored (due to the forcing of defining alg in the header).

I'm happy to put up a PR if you want but I don't have strong opinions about what the right solution is,
eg: removing SignOptions.algorithm or using Partial<JwtHeader> in the SignOptions and correcting the jsonwebtoken package.

Copy link
Copy Markdown
Contributor Author

@nandi95 nandi95 Sep 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alg is a required attribute according to the rfc

Parameter MUST be present and MUST be understood and processed by
implementations

If the changes were not versioned according to semantic versioning, then it must have been a mistake, not intentional.

I'm confused, is this SignOptions is in auth0/node-jsonwebtoken because that would be an implementation and surely this issue is on their side? I could be wrong, I would imagine the auth0 guys would know their stuff.

For now I would assume that SignOptions needs to be updated.

Copy link
Copy Markdown

@randonia randonia Sep 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, it's how they implemented it.

I think there are a couple ambiguity problems-

  1. jsonwebtoken providing undocumented/unclear behavior when specifying algorithm (their README/Docs don't explain how it handles conflicts - eg if SignOptions.algorithm = 'RS256' but SignOptions.header.alg = 'HS256') (seemingly accidentally)
  2. @types/jsonwebtoken introducing a change that breaks existing implementations (see my case where SignOptions.algorithm is defined but JwtHeader.alg is not) since changing from object to JwtHeader in the SignOptions.header type, but only appears as a patch version8.5.1->8.5.2 (also seemingly accidentally)

My personal opinion is that there should only be 1 place to specify Algorithm. If I had my way, I'd agree with you to remove algorithm from SignOptions, fix the jsonwebtoken implementation linked above, and keep it in JwtHeader since it seems that is the least ambiguous path forward. That's a big change that affects both of these projects though, so I didn't put up a PR since that's just my opinion.

For clarity: I perceived that both of these were just accidents, hopefully my tone didn't come off as negative or accusatory :)

Copy link
Copy Markdown

@hiranya911 hiranya911 Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just got bitten by this. We had a package-lock file in place which was pulling in 8.5.1, so we didn't notice it until today when we finally updated the lock file. Thanks for this comment trail. It helped us understand the issue and implement a temporary fix.

encoding?: string;
}

Expand Down Expand Up @@ -98,24 +99,48 @@ export type VerifyErrors =
| JsonWebTokenError
| NotBeforeError
| TokenExpiredError;
export type VerifyCallback = (
export type VerifyCallback<T = JwtPayload> = (
err: VerifyErrors | null,
decoded: object | undefined,
decoded: T | undefined,
) => void;

export type SignCallback = (
err: Error | null, encoded: string | undefined
) => void;

// standard names https://www.rfc-editor.org/rfc/rfc7515.html#section-4.1
export interface JwtHeader {
alg: string;
alg: string | Algorithm;
typ?: string;
cty?: string;
crit?: Array<string | Exclude<keyof JwtHeader, 'crit'>>;
kid?: string;
jku?: string;
x5u?: string;
x5u?: string | string[];
'x5t#S256'?: string;
x5t?: string;
x5c?: string | string[];
}

// standard claims https://datatracker.ietf.org/doc/html/rfc7519#section-4.1
export interface JwtPayload {
[key: string]: any;
iss?: string;
sub?: string;
aud?: string | string[];
exp?: number;
nbf?: number;
iat?: number;
jti?: string;
}

export interface Jwt {
header: JwtHeader;
payload: JwtPayload;
signature: string;
}

// https://github.com/auth0/node-jsonwebtoken#algorithms-supported
export type Algorithm =
"HS256" | "HS384" | "HS512" |
"RS256" | "RS384" | "RS512" |
Expand Down Expand Up @@ -177,7 +202,8 @@ export function sign(
* [options] - Options for the verification
* returns - The decoded token.
*/
export function verify(token: string, secretOrPublicKey: Secret, options?: VerifyOptions): object | string;
export function verify(token: string, secretOrPublicKey: Secret, options: VerifyOptions & { complete: true }): Jwt | string;
export function verify(token: string, secretOrPublicKey: Secret, options?: VerifyOptions): JwtPayload | string;

/**
* Asynchronously verify given token using a secret or a public key to get a decoded token
Expand All @@ -193,6 +219,12 @@ export function verify(
secretOrPublicKey: Secret | GetPublicKeyOrSecret,
callback?: VerifyCallback,
): void;
export function verify(
token: string,
secretOrPublicKey: Secret | GetPublicKeyOrSecret,
options?: VerifyOptions & { complete: true },
callback?: VerifyCallback<Jwt>,
): void;
export function verify(
token: string,
secretOrPublicKey: Secret | GetPublicKeyOrSecret,
Expand All @@ -206,5 +238,5 @@ export function verify(
* [options] - Options for decoding
* returns - The decoded Token
*/
export function decode(token: string, options: DecodeOptions & { json: true } | DecodeOptions & { complete: true }): null | { [key: string]: any };
export function decode(token: string, options?: DecodeOptions): null | { [key: string]: any } | string;
export function decode(token: string, options: DecodeOptions & { json: true } | DecodeOptions & { complete: true }): null | Jwt;
export function decode(token: string, options?: DecodeOptions): null | JwtPayload | string;
Comment on lines +241 to +242
Copy link
Copy Markdown
Contributor Author

@nandi95 nandi95 May 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps an optional type argument could be considered too?

Suggested change
export function decode(token: string, options: DecodeOptions & { json: true } | DecodeOptions & { complete: true }): null | Jwt;
export function decode(token: string, options?: DecodeOptions): null | JwtPayload | string;
export function decode<T = Jwt>(token: string, options: DecodeOptions & { json: true } | DecodeOpti
export function decode<T = JwtPayload>(token: string, options?: DecodeOptions): null | T | string;

Copy link
Copy Markdown
Contributor Author

@nandi95 nandi95 Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or extends rather than =
but anyhow that would violate https://github.com/Microsoft/dtslint/blob/master/docs/no-unnecessary-generics.md :(

Copy link
Copy Markdown
Contributor

@LinusU LinusU Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A type argument has come up before and shouldn't be added because of the following reason in the link you posted:

getMeAT<T>(): T:
If a type parameter does not appear in the types of any parameters, you don't really have a generic function, you just have a disguised type assertion.
Prefer to use a real type assertion, e.g. getMeAT() as number.
Example where a type parameter is acceptable: function id<T>(value: T): T;.
Example where it is not acceptable: function parseJson<T>(json: string): T;.
Exception: new Map<string, number>() is OK.

This is because otherwise you'll write code that assumes that all the tokens passed in are well formed which is bad since the tokens are usually passed from other systems or third parties...

50 changes: 45 additions & 5 deletions types/jsonwebtoken/jsonwebtoken-tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,46 @@ jwt.verify(token, cert, { ignoreExpiration: true }, (err, decoded) => {
// if ignoreExpration == false and token is expired, err == expired token
});

cert = fs.readFileSync("public.pem"); // get public key
jwt.verify(token, cert, (_err, payload) => {
if (payload) {
// $ExpectType JwtPayload
payload;
}
});

cert = fs.readFileSync("public.pem"); // get public key
jwt.verify(token, cert, {}, (_err, payload) => {
if (payload) {
// $ExpectType JwtPayload
payload;
}
});

cert = fs.readFileSync("public.pem"); // get public key
jwt.verify(token, cert, { complete: true }, (_err, payload) => {
if (payload) {
// $ExpectType Jwt
payload;
}
});

cert = fs.readFileSync("public.pem"); // get public key
const verified = jwt.verify(token, cert);

if (typeof verified !== 'string') {
// $ExpectType JwtPayload
verified;
}

cert = fs.readFileSync("public.pem"); // get public key
const verified2 = jwt.verify(token, cert, { complete: true });

if (typeof verified2 !== 'string') {
// $ExpectType Jwt
verified2;
}

/**
* jwt.decode
* https://github.com/auth0/node-jsonwebtoken#jwtdecodetoken
Expand All @@ -160,12 +200,12 @@ decoded = jwt.decode(token, { complete: false, json: false });

decoded = jwt.decode(token, { json: true });
if (decoded) {
// $ExpectType { [key: string]: any; }
// $ExpectType JwtPayload
decoded;
}

decoded = jwt.decode(token, { complete: true });
if (decoded) {
// $ExpectType { [key: string]: any; }
decoded;
const decoded2 = jwt.decode(token, { complete: true });
if (decoded2) {
// $ExpectType Jwt
Comment thread
nandi95 marked this conversation as resolved.
decoded2;
}