feat(pq-jws/ts): phase 2 - implement compact jws core utilities (ENG-1641)#19
feat(pq-jws/ts): phase 2 - implement compact jws core utilities (ENG-1641)#19eacet wants to merge 2 commits intofeature/eng-1640from
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Greptile SummaryThis PR implements phase 2 of the compact JWS functionality, adding comprehensive base64url encoding/decoding and JWS compact format parsing with strict validation. Key Changes:
Minor Observations:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Start([verifyJwsCompact called]) --> Parse[parseJwsCompact]
Parse --> ValidateFormat{Validate format:<br/>3 segments,<br/>non-empty header/signature}
ValidateFormat -->|Invalid| FormatError[Throw JwsFormatError]
ValidateFormat -->|Valid| DecodeHeader[Decode base64url header]
DecodeHeader --> ValidateHeader{Validate header:<br/>UTF-8, JSON,<br/>required fields,<br/>no duplicates}
ValidateHeader -->|Invalid| ValidationError[Throw JwsValidationError]
ValidateHeader -->|Valid| DecodePayload[Decode base64url payload]
DecodePayload --> DecodeSignature[Decode base64url signature]
DecodeSignature --> CheckBounds{Check size bounds:<br/>header, payload,<br/>signature lengths}
CheckBounds -->|Exceeded| ValidationError
CheckBounds -->|Within limits| BuildSigningInput[Build signing input:<br/>header.payload]
BuildSigningInput --> CallVerifier[Call verifier callback<br/>with signing input + signature]
CallVerifier --> CheckResult{Verifier returns<br/>boolean?}
CheckResult -->|No| ValidationError
CheckResult -->|Yes| ReturnResult([Return verification result])
style Start fill:#e1f5ff
style ReturnResult fill:#e1f5ff
style FormatError fill:#ffebee
style ValidationError fill:#ffebee
style CallVerifier fill:#f3e5f5
Last reviewed commit: f56a36e |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9aac90db59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
|
|
||
| function maxEncodedLength(decodedLength: number): number { | ||
| return Math.ceil(decodedLength / 3) * 4; |
There was a problem hiding this comment.
resolveJwsCompactParseOptions currently treats each segment’s worst-case encoded size as padded Base64 (Math.ceil(n/3)*4), but compact JWS uses unpadded base64url, so this overestimates by up to 2 chars per segment and rejects valid tight configurations. For example, options that are valid under unpadded encoding (and would parse correctly) can fail early with “internally inconsistent” purely because of this padded-length math, which makes size hardening with custom limits unnecessarily unusable.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export interface JwsCompactParseOptions { | ||
| maxCompactLength: number; | ||
| maxHeaderLength: number; | ||
| maxPayloadLength: number; | ||
| maxSignatureLength: number; | ||
| } | ||
|
|
||
| export type JwsCompactParseOptionsInput = Partial<JwsCompactParseOptions>; | ||
|
|
||
| export interface VerifyJwsCompactOptions { | ||
| parseOptions?: JwsCompactParseOptionsInput; | ||
| } |
There was a problem hiding this comment.
Consider using type instead of interface for these simple data structures that don't use inheritance or extension, per project conventions. For example:
| export interface JwsCompactParseOptions { | |
| maxCompactLength: number; | |
| maxHeaderLength: number; | |
| maxPayloadLength: number; | |
| maxSignatureLength: number; | |
| } | |
| export type JwsCompactParseOptionsInput = Partial<JwsCompactParseOptions>; | |
| export interface VerifyJwsCompactOptions { | |
| parseOptions?: JwsCompactParseOptionsInput; | |
| } | |
| export type JwsCompactParseOptions = { | |
| maxCompactLength: number; | |
| maxHeaderLength: number; | |
| maxPayloadLength: number; | |
| maxSignatureLength: number; | |
| }; | |
| export type JwsCompactParseOptionsInput = Partial<JwsCompactParseOptions>; | |
| export type VerifyJwsCompactOptions = { | |
| parseOptions?: JwsCompactParseOptionsInput; | |
| }; |
Context Used: Rule from dashboard - Use type by default in TypeScript unless you specifically need interface features like extension... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/pq-jws/ts/src/types.ts
Line: 54-65
Comment:
Consider using `type` instead of `interface` for these simple data structures that don't use inheritance or extension, per project conventions. For example:
```suggestion
export type JwsCompactParseOptions = {
maxCompactLength: number;
maxHeaderLength: number;
maxPayloadLength: number;
maxSignatureLength: number;
};
export type JwsCompactParseOptionsInput = Partial<JwsCompactParseOptions>;
export type VerifyJwsCompactOptions = {
parseOptions?: JwsCompactParseOptionsInput;
};
```
**Context Used:** Rule from `dashboard` - Use `type` by default in TypeScript unless you specifically need `interface` features like extension... ([source](https://app.greptile.com/review/custom-context?memory=c862f053-5655-4b41-be69-c840e3c9f280))
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Context Used: Rule from Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/pq-jws/ts/src/types.ts
Line: 1-52
Comment:
Most of these interfaces could be converted to `type` declarations per project style preferences, except `JwsVerifierContext` which uses `extends` (though it could also be simplified to `type JwsVerifierContext = JwsSignerContext`).
**Context Used:** Rule from `dashboard` - Use `type` instead of `interface` for DTOs and simple data structures that don't use inheritance or ... ([source](https://app.greptile.com/review/custom-context?memory=2b2a7a55-162e-44b9-8c4c-3f52514f7037))
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
1 participant
Summary
Package(s)
Languages
Checklist
biome check,cargo fmt)Related Issues