fix(platform): support native shell auth callbacks#372
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
HamedMP
changed the title
Greptile SummaryThis PR adds a signed native redirect handoff (
Confidence Score: 5/5Safe to merge; the HMAC binding is consistent at signing and verification, HTML escaping is applied at every embedding point, and the regex change is narrowly scoped to session names. The signing and verification functions both normalize the redirect URI before computing the HMAC, so there is no canonicalization mismatch. The approval page re-validates the signature on GET before embedding hidden fields, and the POST handler re-validates it again before rendering the success page. The only finding is a swapped argument order in the timingSafeTokenEquals call that is functionally harmless today. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as macOS Shell App
participant Platform as Platform API
participant Browser as User Browser
participant Clerk as Clerk Auth
App->>Platform: POST /api/auth/device/code
Platform->>Platform: normalizeNativeRedirectUri + signNativeRedirectUri
Platform-->>App: verificationUri with redirect_uri and redirect_sig
App->>Browser: Open verificationUri
Browser->>Platform: GET /auth/device with redirect_uri and redirect_sig
Platform->>Platform: verifiedNativeRedirectUri HMAC check
Platform-->>Browser: Approval HTML with hidden redirect fields
Browser->>Clerk: Sign-in
Clerk-->>Browser: Session token
Browser->>Platform: POST /auth/device/approve with redirectUri and redirectSig
Platform->>Platform: Re-validate HMAC then approveDeviceCode
Platform-->>Browser: Success page with meta refresh to matrixos://auth
Browser->>App: matrixos:// URL scheme handoff
App->>Platform: Poll GET /api/auth/device/token
Platform-->>App: accessToken
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/platform/src/auth-routes.ts:126-127
The arguments to `timingSafeTokenEquals` are swapped relative to the function's parameter semantics. The function signature is `(actual: string | undefined, expected: string)`, where `actual` carries the potentially-absent caller value and `expected` is the computed reference. Here the locally-named `expected` (computed HMAC, always defined) is passed as `actual`, and the user-supplied `signatureValue` is passed as `expected`. The result is correct today because HMAC-SHA256 always returns a non-empty string, so the `!actual` early-exit guard is effectively dead for this call. Swapping the order aligns the call with the function's documented intent and makes it clear which side the undefined-guard protects.
```suggestion
const expected = signNativeRedirectUri(secret, userCode, redirectUri);
return timingSafeTokenEquals(signatureValue, expected) ? redirectUri : null;
```
Reviews (3): Last reviewed commit: "fix(platform): bind native auth callback..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const form = await c.req.parseBody(); | ||
| formCsrf = typeof form.csrf === 'string' ? form.csrf : undefined; | ||
| userCode = typeof form.userCode === 'string' ? form.userCode : undefined; | ||
| nativeRedirectUri = normalizeNativeRedirectUri(form.redirectUri); |
There was a problem hiding this comment.
clientId gate only enforced at code issuance
The PR description states the native callback "is only accepted for clientId=matrix-os-macos", but the enforcement is only in the POST /api/auth/device/code route (line 713). The GET /auth/device approval page and the POST /auth/device/approve handler both accept any matrixos://auth redirect URI without checking that the originating clientId was matrix-os-macos. A CLI client or any other clientId that manually appends ?redirect_uri=matrixos://auth to the verification URL would get the native redirect embedded in the approval page and trigger a matrixos:// callback on success.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/platform/src/auth-routes.ts
Line: 837
Comment:
**`clientId` gate only enforced at code issuance**
The PR description states the native callback "is only accepted for `clientId=matrix-os-macos`", but the enforcement is only in the `POST /api/auth/device/code` route (line 713). The `GET /auth/device` approval page and the `POST /auth/device/approve` handler both accept any `matrixos://auth` redirect URI without checking that the originating `clientId` was `matrix-os-macos`. A CLI client or any other `clientId` that manually appends `?redirect_uri=matrixos://auth` to the verification URL would get the native redirect embedded in the approval page and trigger a `matrixos://` callback on success.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
[codex] Fixed in eef92cb. Native redirect callbacks are now signed when the macOS device code is issued, and both the approval page and approval POST ignore unsigned/manual redirect_uri values.
| expect(validateSessionName("matrix-sess_abc123")).toBe("matrix-sess_abc123"); | ||
| expect(validateLayoutName("dev-workspace-1")).toBe("dev-workspace-1"); |
There was a problem hiding this comment.
The regex change expands the max allowed session name from 31 to 64 characters, but the existing "oversized" test only covers 65-char strings (which exceed both old and new limits). There is no acceptance test for a 64-char session name, so a future tightening of the regex might regress silently.
| expect(validateSessionName("matrix-sess_abc123")).toBe("matrix-sess_abc123"); | |
| expect(validateLayoutName("dev-workspace-1")).toBe("dev-workspace-1"); | |
| expect(validateSessionName("matrix-sess_abc123")).toBe("matrix-sess_abc123"); | |
| expect(validateSessionName("a".repeat(64))).toBe("a".repeat(64)); | |
| expect(validateLayoutName("dev-workspace-1")).toBe("dev-workspace-1"); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/gateway/shell-names.test.ts
Line: 28-29
Comment:
**New session-name length boundary (64) is untested in the positive direction**
The regex change expands the max allowed session name from 31 to 64 characters, but the existing "oversized" test only covers 65-char strings (which exceed both old and new limits). There is no acceptance test for a 64-char session name, so a future tightening of the regex might regress silently.
```suggestion
expect(validateSessionName("matrix-sess_abc123")).toBe("matrix-sess_abc123");
expect(validateSessionName("a".repeat(64))).toBe("a".repeat(64));
expect(validateLayoutName("dev-workspace-1")).toBe("dev-workspace-1");
```
How can I resolve this? If you propose a fix, please make it concise.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!
There was a problem hiding this comment.
[codex] Fixed in eef92cb by adding an explicit positive 64-character session-name boundary assertion.
HamedMP
force-pushed
the
087-native-auth-callback
branch
from
4336e1d to
922b201
Compare
1 participant
Summary
matrixos://authredirect handoff to the device approval flow for the macOS shell clientInvariants
clientId=matrix-os-macoswithmatrixos://auth.Validation
pnpm test tests/platform/device-routes.test.tspnpm test tests/gateway/shell-names.test.tspnpm run check:patterns(0 violations; existing warning buckets only)