Repository navigation
Validate the OAuth state timestamp instead of relying on a loose comparison - #82
Merged
diwenne merged 1 commit intoOct 5, 2026
Merged
Conversation
|
@Davidowi2 is attempting to deploy a commit to the diwenne's projects Team on Vercel. A member of the Team first needs to authorize it. |
Davidowi2
commented
Oct 2, 2026
| return null; | ||
| } | ||
|
|
||
| // Reject a missing or non-numeric timestamp. `Date.now() - "abc"` is NaN, |
Author
There was a problem hiding this comment.
Date.now() - "not-a-number" is NaN, and every comparison against NaN is false, so a non-numeric ts slipped through and never expired.
Davidowi2
commented
Oct 2, 2026
| // rejected for the same reason: it has not aged at all. | ||
| if (typeof parsed.ts !== "number" || !Number.isFinite(parsed.ts)) { | ||
| return null; | ||
| } |
Author
There was a problem hiding this comment.
A ts in the future gives a negative age, which also passes a > MAX check. Same hole, other direction.
Davidowi2
commented
Oct 2, 2026
| expect(verifyOAuthState(`${state}tampered`)).toBeNull(); | ||
| }); | ||
|
|
||
| // Builds a state whose signature is valid but whose payload has an |
Author
There was a problem hiding this comment.
Valid signature on purpose, so each case hits the payload checks instead of stopping at the HMAC.
…arison
verifyOAuthState only checked the upper bound of the state age:
if (!parsed.workspaceId || Date.now() - parsed.ts > STATE_MAX_AGE_MS) {
Two payload shapes slip past that check.
A non-numeric `ts` makes `Date.now() - parsed.ts` evaluate to NaN, and every
comparison against NaN is false, so a state carrying `ts: "not-a-number"` is
accepted and never expires. A `ts` in the future produces a negative age,
which is also never greater than the maximum, so that state is accepted for
the same reason.
`workspaceId` was checked for truthiness only, so a number, boolean, array or
object was accepted and returned as `OAuthStatePayload`, whose type declares
it as a string.
This validates the timestamp is a finite number, rejects an age outside
[0, STATE_MAX_AGE_MS], and requires workspaceId to be a non-empty string.
The signature check is unchanged: every case here still needs a valid HMAC.
Davidowi2
force-pushed
the
fix/oauth-state-timestamp-validation
branch
from
October 2, 2026 17:20
30aefe8 to
5860878
Compare
This was referenced Oct 2, 2026
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
2 participants
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.
verifyOAuthStateonly checked the upper bound of the state age:Two payload shapes slip past that check.
A non-numeric
tsmakesDate.now() - parsed.tsevaluate toNaN, and every comparison againstNaNis false, so a state carryingts: "not-a-number"is accepted and never expires. Atsin the future produces a negative age, which is also never greater than the maximum, so that state is accepted for the same reason.workspaceIdwas checked for truthiness only, so a number, boolean, array or object was accepted and returned asOAuthStatePayload, whose type declares it as a string.This validates that the timestamp is a finite number, rejects an age outside
[0, STATE_MAX_AGE_MS], and requiresworkspaceIdto be a non-empty string.The signature check is unchanged: every case here still needs a valid HMAC, so this is input validation on already-authenticated data rather than a bypass.
Verification
npm test— 314 passed (309 before, plus 5 new)npm run lint— cleanPre-existing failure
npm run typecheckandnpm run buildboth fail onmainas well, on the same unrelated line:That is untouched by this change — I confirmed it by stashing this branch and re-running both commands on a clean
main.