Skip to content

fix: add JWT token expiry and replace shell exec with spawn in git routes#88

Merged
bbsngg merged 3 commits intoOpenLAIR:mainfrom
haoyu-haoyu:fix/jwt-expiry-and-git-exec-safety
Apr 6, 2026
Merged

fix: add JWT token expiry and replace shell exec with spawn in git routes#88
bbsngg merged 3 commits intoOpenLAIR:mainfrom
haoyu-haoyu:fix/jwt-expiry-and-git-exec-safety

Conversation

@haoyu-haoyu
Copy link
Copy Markdown
Contributor

@haoyu-haoyu haoyu-haoyu commented Mar 24, 2026

Summary

Two security hardening improvements.

1. JWT tokens now expire (default: 7 days)

Previously, generateToken() issued tokens with no expiresIn option — once issued, a token was valid forever. A leaked token (e.g., from browser localStorage, logs, or a shared machine) would grant permanent access.

Now tokens expire after 7 days by default. Configurable via JWT_EXPIRY environment variable (accepts any ms-compatible duration string, e.g., "24h", "30d").

// Before:
jwt.sign({ userId, username }, JWT_SECRET)

// After:
jwt.sign({ userId, username }, JWT_SECRET, { expiresIn: JWT_EXPIRY })

2. Git routes: execAsync (shell=true) → spawnAsync (shell=false)

6 call sites in server/routes/git.js used execAsync() which internally uses child_process.exec with shell: true. Two of these interpolated user-influenced values (commit hashes) directly into shell command strings:

// Before (shell=true, commit hash interpolated):
await execAsync(`git show --stat --format='' ${commit.hash}`, { cwd })
await execAsync(`git show ${commit}`, { cwd })

Replaced all 6 with spawnAsync() which passes arguments as an array, avoiding shell interpretation:

// After (shell=false, arguments as array):
await spawnAsync('git', ['show', '--stat', '--format=', commit.hash], { cwd })
await spawnAsync('git', ['show', commit], { cwd })

Removed the now-unused exec and promisify imports.

Test plan

  • Login → verify token works for 7 days
  • Git status, branches, commits, diffs all still render correctly
  • Set JWT_EXPIRY=1h → verify token expires after 1 hour

Zhang-Henry
Zhang-Henry reviewed Apr 6, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Zhang-Henry Zhang-Henry left a comment

Choose a reason for hiding this comment

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

Code Review

Two independent changes: (1) configurable JWT expiry, and (2) exec → spawn migration in git routes to eliminate shell injection. Both directions are correct and necessary.

Critical — Verify Migration Completeness

The most important thing to confirm before merging: The diff claims all execAsync calls in git.js are converted to spawnAsync, but the full file on the repo shows several execAsync calls still present. This may be a branch sync issue. The following lines must be confirmed as migrated on the PR branch:

Location Risk
GET /commit-diff (~L708-711) Shell injection — req.query.commit is a raw query parameter interpolated into a shell string. GET /commit-diff?commit=abc;rm -rf / would execute arbitrary commands.
GET /commits (~L679-682) commit.hash from git log output interpolated into shell string
validateGitRepository (~L100, ~L107) Lower risk but still shell-interpolated
GET /status (~L277) Lower risk
GET /branches (~L568) Lower risk

Do not merge until all execAsync usages are confirmed gone on the PR branch.

JWT Changes (server/middleware/auth.js)

The expiry addition is correct. Minor notes:

  • JWT_EXPIRY is not validated at startup — a misconfigured value like JWT_EXPIRY=invalid will crash the login endpoint at sign-time, not at startup. Consider adding early validation.
  • Hardcoded fallback secret ('claude-ui-dev-secret-change-in-production') pre-dates this PR but is worth noting — tokens signed with this known-public secret are trivially forgeable. Should log a loud warning if JWT_SECRET is not set in non-dev environments.
  • The authenticateToken middleware returns a generic 401 for both expired and invalid tokens. A follow-up could distinguish TokenExpiredError to help clients decide whether to re-login vs. show an error.

spawnAsync Implementation

The wrapper is well-structured. Notes:

  • git diff and git status return exit code 1 when changes exist (not an error). spawnAsync rejects on non-zero exit, matching prior execAsync behavior. This is a latent issue worth documenting — not a regression.
  • No timeout on spawnAsync — a hung git operation (SSH push waiting for credentials) will block the route handler indefinitely. Same as before but worth a follow-up.

Minor

  • server/routes/git.js, generateCommitMessageWithAI (~L815-872): Contains 10+ console.log calls with emoji prefixes that are clearly debug logs. Not introduced by this PR, but if the surrounding area is being touched, it's a good time to clean up.
  • If the migration is complete, confirm the exec import from child_process and promisify from util are removed.

@haoyu-haoyu haoyu-haoyu force-pushed the fix/jwt-expiry-and-git-exec-safety branch from 1b6b8ac to a111d4f Compare April 6, 2026 12:57
@haoyu-haoyu
Copy link
Copy Markdown
Contributor Author

haoyu-haoyu commented Apr 6, 2026

@Zhang-Henry Thank you for the review! Updates pushed:

exec → spawn migration

All execAsync in server/routes/git.js are fully migrated (33 spawnAsync, 0 execAsync). The exec and promisify imports have been removed.

Scope note: execAsync usage in other files (cli-chat.js, local-gpu.js, openrouter.js, user.js, gitConfig.js) is intentionally out of scope for this PR — those files are not in the git routes request path and don't handle user-supplied input in the same way. Happy to address them in a follow-up if desired.

JWT improvements

  • JWT_EXPIRY validated at startup — test-signs a token immediately; crashes with a clear error on misconfiguration instead of failing at first login
  • Production warning for default JWT_SECRET — logs [SECURITY] warning when NODE_ENV=production and JWT_SECRET is unset

haoyu-haoyu and others added 3 commits April 6, 2026 13:52
@haoyu-haoyu @bbsngg
fix: add JWT token expiry and replace shell exec with spawn in git ro…
0376bee 
…utes

JWT tokens:
- Add configurable expiry (default 7 days) via JWT_EXPIRY env var
- Previously tokens never expired, meaning a leaked token grants
  permanent access until the user record is deleted

Git routes:
- Replace all execAsync (shell=true) calls with spawnAsync (shell=false)
- execAsync passes commands through the system shell, which could
  interpret metacharacters in paths or commit hashes
- spawnAsync passes arguments as an array, avoiding shell interpretation
- Remove unused exec/promisify imports

Affected call sites in git.js:
- git rev-parse --is-inside-work-tree
- git rev-parse --show-toplevel
- git status --porcelain
- git branch -a
- git show --stat (commit hash was interpolated into shell string)
- git show <commit> (commit hash was interpolated into shell string)
@haoyu-haoyu @bbsngg
fix: add JWT_EXPIRY startup validation and default-secret warning
b146276 
- Validate JWT_EXPIRY at startup by attempting a test sign — crashes
  immediately on misconfiguration instead of at first login attempt
- Warn loudly when JWT_SECRET is not set in production environment
@bbsngg @claude
fix: change default JWT token expiry from 7 days to 30 days
844363e 
Reduce friction for users who may forget credentials with shorter expiry.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@bbsngg bbsngg force-pushed the fix/jwt-expiry-and-git-exec-safety branch from a111d4f to 844363e Compare April 6, 2026 17:55
@bbsngg bbsngg merged commit 1ae2a19 into OpenLAIR:main Apr 6, 2026
1 check passed
@haoyu-haoyu haoyu-haoyu mentioned this pull request Apr 6, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Zhang-Henry Zhang-Henry Zhang-Henry left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@haoyu-haoyu @Zhang-Henry @bbsngg