fix(auth): address PR #965 review comments and improve coverage#968
fix(auth): address PR #965 review comments and improve coverage#968vdimarco wants to merge 1 commit into
Conversation
- Add HTTPS enforcement for non-localhost callbacks to prevent token leakage
- Add stricter token validation: reject empty/whitespace tokens (!token || !token.trim())
- Add navigate.test.ts for the extracted navigateTo utility
- Add new tests: whitespace-only token, null token, malformed URLs,
wildcard subdomains, non-JSON API errors, missing email, idle state
- Improve abort test fidelity with DOMException('AbortError')
- Add docstring for getAllowedDomains explaining function vs const choice
- Total: 37 tests (36 page + 1 navigate), all passing
https://claude.ai/code/session_01CKCwH6XgHopRssntWSgViq
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a unit test for the navigateTo function, expands Terragon auth page tests to cover callback URL HTTPS enforcement for non-localhost domains, cached credentials fallback logic, and whitespace-only token validation. Modifies the Terragon auth page to enforce HTTPS for non-localhost callback URLs and tighten token validation to reject empty or whitespace-only tokens. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: e42bfe7519
ℹ️ 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".
| const { token } = await response.json(); | ||
|
|
||
| if (!token) { | ||
| if (!token || !token.trim()) { | ||
| throw new Error( |
There was a problem hiding this comment.
If /api/terragon/auth returns a non-string token (e.g., null, number, or object), token.trim() will throw a TypeError, which is then surfaced to users as token.trim is not a function via the error message. That’s a regression from the prior !token check and makes a server-side shape bug present as a confusing client error. Consider validating typeof token === "string" before trimming so non-string tokens become a clean, user-friendly error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR tightens the Terragon auth bridge’s redirect and token-handling logic to reduce open-redirect/token-leak risk, while expanding Jest coverage around the auth flow and redirect helper.
Changes:
- Enforce HTTPS for non-localhost callback URLs and improve allowlist helper documentation.
- Strengthen token validation to reject empty/whitespace tokens and adjust the surfaced error message.
- Add/adjust Jest tests for callback validation, token edge cases, and the extracted
navigateTohelper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/app/auth/terragon/page.tsx |
Adds HTTPS enforcement for callbacks and stricter token validation/error messaging. |
src/app/auth/terragon/__tests__/page.test.tsx |
Expands coverage for HTTP callback rejection, token edge cases, and auth-error fallback behavior. |
src/app/auth/terragon/__tests__/navigate.test.ts |
Introduces a new unit test for the navigateTo redirect helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { token } = await response.json(); | ||
|
|
||
| if (!token) { | ||
| if (!token || !token.trim()) { | ||
| throw new Error( | ||
| "Auth endpoint returned an empty token. Please try again." | ||
| "Server returned an empty auth token. Please try again." | ||
| ); | ||
| } |
There was a problem hiding this comment.
token.trim() assumes token is a string. If the API ever returns a non-string truthy value (e.g., { token: 123 }), this will throw a TypeError and surface as a generic auth failure. Consider validating typeof token === "string" (and trimming) before using it, and treat any non-string as an empty/invalid token.
|
|
||
| // JSDOM doesn't truly navigate, but we can verify the assignment | ||
| // doesn't throw and the function is callable | ||
| expect(() => navigateTo(testUrl)).not.toThrow(); |
There was a problem hiding this comment.
This test declares originalHref but never uses it, which will fail next lint under the default unused-vars rules. Either remove it or use it to restore window.location.href after the test.
| expect(() => navigateTo(testUrl)).not.toThrow(); | |
| try { | |
| expect(() => navigateTo(testUrl)).not.toThrow(); | |
| } finally { | |
| // Restore the original location to avoid leaking state between tests | |
| window.location.href = originalHref; | |
| } |
| // JSDOM doesn't truly navigate, but we can verify the assignment | ||
| // doesn't throw and the function is callable | ||
| expect(() => navigateTo(testUrl)).not.toThrow(); | ||
|
|
||
| // In JSDOM, setting location.href to a full URL may or may not | ||
| // update the property. The key contract is that the function | ||
| // calls window.location.href = url without throwing. | ||
| }); |
There was a problem hiding this comment.
The test currently only asserts that navigateTo(testUrl) doesn't throw, but it doesn't verify the core contract (that the function attempts to set window.location.href). Consider spying on/mocking the location.href setter (or temporarily replacing window.location) so you can assert the assignment without relying on JSDOM navigation behavior, and restore the original afterwards to avoid cross-test leakage.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
wildcard subdomains, non-JSON API errors, missing email, idle state
https://claude.ai/code/session_01CKCwH6XgHopRssntWSgViq
Greptile Overview
Greptile Summary
This PR tightens the Terragon auth-bridge redirect flow by enforcing HTTPS for non-localhost callback URLs, rejecting empty/whitespace tokens, and expanding test coverage around callback validation (malformed URLs, wildcard subdomains, missing email, non-JSON errors, abort handling). It also adds unit tests for the extracted
navigateToutility to ensure navigation behavior is validated independently from the page-level flow.Confidence Score: 0/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant B as Browser participant P as TerragonAuthPage participant U as navigateTo() participant A as Terragon API B->>P: Load /auth/terragon P->>P: Parse callback + token params P->>P: Validate token (!token || !token.trim()) alt Invalid token P-->>B: Render error/idle else Valid token P->>P: Validate callback URL alt Non-localhost callback P->>P: Enforce https end P->>P: Check allowed domains (wildcards) alt Callback not allowed P-->>B: Render error else Allowed P->>A: POST /auth/bridge (with AbortSignal) alt API returns success P->>U: navigateTo(callback) U-->>B: window.location.assign / router push else API returns non-JSON or error P-->>B: Render error end end end note over P,A: Abort -> DOMException('AbortError') handled in testsContext used:
dashboard- CLAUDE.md (source)Summary by CodeRabbit
Release Notes
Bug Fixes
Tests