Skip to content

fix(draft): fail closed when the submission-token expires_at is unparseable#1713

Merged
JSONbored merged 2 commits into
JSONbored:mainfrom
RenzoMXD:fix/draft-token-expiry-fail-closed
Jun 29, 2026
Merged

fix(draft): fail closed when the submission-token expires_at is unparseable#1713
JSONbored merged 2 commits into
JSONbored:mainfrom
RenzoMXD:fix/draft-token-expiry-fail-closed

Conversation

@RenzoMXD

Copy link
Copy Markdown
Contributor

Summary

processSubmitDraft (src/services/draft.ts) gates the contributor content-PR flow on the stored user token's expiry:

if (!tokenRow || tokenRow.consumed_at || new Date(tokenRow.expires_at).getTime() < Date.now() || !encKey) {

expires_at is a TEXT NOT NULL column (migration 0048_drafts.sql), so SQLite does not enforce ISO-8601 shape. A corrupted/partial write, a schema migration, or any future path that stores a non-ISO value makes new Date(...).getTime() return NaN, and NaN < Date.now() is false -- the token is then treated as not expired, and the submission proceeds with a possibly-stale/invalid user token.

Fix

Mirror src/auth/security.ts's fail-closed session-expiry guard, which carries an explicit comment naming this exact failure mode:

// Fail closed on an unparseable expiry: Date.parse -> NaN makes `NaN <= Date.now()` false...
const expiresAtMs = Date.parse(session.expiresAt);
if (session.revokedAt || !Number.isFinite(expiresAtMs) || expiresAtMs <= Date.now()) return null;

Compute expiresAtMs with Date.parse and treat a non-finite value as expired, so an unparseable expires_at short-circuits to the existing token_unavailable arm and never proceeds with the token. No behavior change for any well-formed row.

Scope

Validation

  • git diff --check
  • npm run actionlint
  • npm run db:migrations:check
  • npm run typecheck
  • npm run test:coverage -- test/unit/draft.test.ts 95/95 pass; the changed lines are covered (the existing expired/consumed/no-token tests plus a new malformed-expires_at regression that fails closed to token_unavailable).

Targeted run:

npx vitest run test/unit/draft.test.ts
# 95/95 passed

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • This is an auth/token-expiry boundary change -- the new test covers the negative path (a malformed expires_at now fails closed instead of authenticating). No CORS/cookie/session-shape change; the stored token is only ever consumed when its expiry parses to a finite, future timestamp.
  • No UI changes.
  • No docs/changelog changes.

UI Evidence

Not applicable -- backend token-expiry guard with no visible UI, frontend, docs, or extension surface.

Closes #1712

…seable

processSubmitDraft gated the contributor content-PR flow on
`new Date(tokenRow.expires_at).getTime() < Date.now()`. expires_at is a
TEXT NOT NULL column, so SQLite does not enforce ISO-8601 shape; a
corrupted/partial write or any future path storing a non-ISO value makes
.getTime() return NaN, and `NaN < Date.now()` is false -- the token was
then treated as not expired and the submission proceeded with a possibly
stale user token.

Mirror src/auth/security.ts' fail-closed session-expiry guard: compute
expiresAtMs with Date.parse, treat a non-finite value as expired, so an
unparseable expires_at short-circuits to the existing token_unavailable
arm and never proceeds with the token. No behavior change for any
well-formed row.

Closes JSONbored#1712
@RenzoMXD RenzoMXD requested a review from JSONbored as a code owner June 29, 2026 05:43
@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 29, 2026
@gittensory-orb

gittensory-orb Bot commented Jun 29, 2026

Copy link
Copy Markdown

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-06-29 18:31:47 UTC

2 files · 1 AI reviewer · no blockers · readiness 55/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
This change correctly makes processSubmitDraft fail closed when a stored draft-token expires_at value parses to NaN, routing the draft through the existing token_unavailable path instead of continuing with an invalid expiry. The added regression test exercises the production processSubmitDraft path with a malformed expires_at and verifies no outbound fetch happens before the draft is marked errored. The visible diff is focused and preserves the existing behavior for missing, consumed, expired, and well-formed future token rows.

Nits — 7 non-blocking
  • nit: src/services/draft.ts:678 names new Date(...).getTime() in the new comment even though the implementation now uses Date.parse, so tighten the wording to avoid making readers reconcile two parsing APIs.
  • nit: test/unit/draft.test.ts:1034 restores the global fetch spy only after the assertions, so a thrown assertion can leak the spy into later tests; wrap the spy lifecycle in try/finally or use the file's cleanup convention.
  • src/services/draft.ts:678 can say `Date.parse(...) -> NaN makes comparisons false without the Number.isFinite guard` to match the actual code.
  • test/unit/draft.test.ts:1034 should restore `fetchSpy` in a finally block in this new test, even though the surrounding tests use the same simpler pattern.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #1712
Related work ⚠️ 2 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (size label size:XS; 1 linked issue).
Validation posture ❌ 5/25 Preflight is holding this PR; address the blocker before review.
Contributor workload ✅ 10/10 Author activity: 45 registered-repo PR(s), 20 merged, 9 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RenzoMXD; Gittensor profile; 45 PR(s), 9 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
Contributor next steps
  • Review top overlaps.
  • Add a concise scope and risk note.
  • Fix the blocker.
  • Triage stale or unlinked PRs.
  • Refresh registry data or choose a registered active repo.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@gittensory-orb gittensory-orb Bot added gittensor Gittensor contributor context gittensor:bug Gittensor-scored bug fix - worth 0.5x multiplier. labels Jun 29, 2026
@codecov

codecov Bot commented Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.58%. Comparing base (59cf13f) to head (3ecefd1).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1713   +/-   ##
=======================================
  Coverage   95.58%   95.58%           
=======================================
  Files         204      204           
  Lines       22313    22314    +1     
  Branches     8065     8066    +1     
=======================================
+ Hits        21328    21329    +1     
  Misses        408      408           
  Partials      577      577           
Files with missing lines Coverage Δ
src/services/draft.ts 98.85% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jun 29, 2026
@JSONbored JSONbored merged commit b261e2b into JSONbored:main Jun 29, 2026
15 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jun 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix - worth 0.5x multiplier. gittensor Gittensor contributor context lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(draft): fail closed when the submission-token expires_at is unparseable

2 participants