Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 435de2d The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
🟠⚠️ Major (1) 🟠⚠️
🟠 1) oauth.ts Defense-in-depth regression: removal of redirect URL allowlist
Issue: This PR removes the ALLOWED_REDIRECT_HOSTNAMES allowlist validation that previously restricted OAuth callback redirects to localhost, 127.0.0.1, and *.inkeep.com domains. While the OAuth state parameter (HMAC-SHA256 signed, 10-minute TTL, nonce) remains intact and provides CSRF protection, this removal shifts the security boundary from defense-in-depth (code-enforced domain allowlist + operator-controlled env var) to operator-trust-only (env var alone).
Why: This is a defense-in-depth regression with the following implications:
-
Not a classic open redirect — The redirect destination (
INKEEP_AGENTS_MANAGE_UI_URL) is controlled by the server operator via environment variable, not by request parameters. External attackers cannot directly control where users are redirected. -
Operator misconfiguration risk — If an operator sets
INKEEP_AGENTS_MANAGE_UI_URLto a malicious or attacker-controlled domain (via env misconfiguration, compromised CI/CD, or supply chain attack), the OAuth callback would redirect users with workspace metadata in the URL. -
Data exposure in redirect — The redirect URL includes
safeWorkspaceData(teamId, teamName, teamDomain, botScopes, installerUserId, connectionId, etc.). WhilebotTokenis excluded, metadata exposure to an untrusted domain is still a concern.
Fix: Consider one of these mitigations based on your security posture:
Option A (Recommended) — Add runtime validation with configurable allowlist:
// New env var for custom deployments
const ALLOWED_REDIRECT_PATTERNS = env.SLACK_OAUTH_REDIRECT_ALLOWLIST
? env.SLACK_OAUTH_REDIRECT_ALLOWLIST.split(',')
: ['localhost', '127.0.0.1', '.inkeep.com'];
function isAllowedRedirectUrl(url: string): boolean {
try {
const parsed = new URL(url);
return ALLOWED_REDIRECT_PATTERNS.some(pattern =>
pattern.startsWith('.')
? parsed.hostname.endsWith(pattern)
: parsed.hostname === pattern
);
} catch {
return false;
}
}
// Log warning instead of throwing to allow custom domains while maintaining visibility
if (!isAllowedRedirectUrl(manageUiUrl)) {
logger.warn({ manageUiUrl }, 'INKEEP_AGENTS_MANAGE_UI_URL is not in default allowlist - ensure this is intentional');
}Option B — Keep removal but add documentation and audit logging:
- Document in
.env.examplethat operators are responsible for securing this URL - Add audit log when OAuth completes with the redirect destination for incident investigation
Option C — Accept the risk explicitly:
- If this is intentional for self-hosted/custom domain support and the operator trust model is acceptable, add a code comment explaining the security trade-off and update the changeset message to document this decision.
Refs:
- oauth.ts:32 — removed validation location
- oauth.ts:392-413 — safeWorkspaceData passed in redirect
- logout/route.ts:19-34 — existing open redirect protection pattern
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
.changeset/mighty-trains-teach.md:5Changeset message doesn't follow project standards
💡 APPROVE WITH SUGGESTIONS
Summary: This PR accomplishes its goal of allowing custom domains for self-hosted deployments, but removes a defense-in-depth security control without adding compensating measures. The change is low-risk for Inkeep-managed deployments but creates operator-trust dependencies for self-hosted scenarios. Consider adding either a configurable allowlist or documentation clarifying the security responsibility shift to operators.
Discarded (0)
| Location | Issue | Reason Discarded |
|---|---|---|
| — | — | No findings were discarded |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
1 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
| Total | 2 | 1 | 0 | 0 | 1 | 0 | 0 |
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (1) Pending | Risk: Medium
Delta Review Summary
This is a re-review scoped to changes since the last automated review. The delta contains only:
.changeset/mighty-trains-teach.md— Updated from informal message to project-standard format ✅
Resolved Feedback:
The prior inline comment about the changeset message not following project standards has been addressed. The message now correctly uses an action verb and describes the change's impact on consumers.
🕐 Pending Recommendations (1)
- 🟠
oauth.tsDefense-in-depth regression: removal of redirect URL allowlist without compensating controls
💡 APPROVE WITH SUGGESTIONS
Summary: The delta changes are clean — the author addressed the changeset message feedback. The prior MAJOR finding regarding the defense-in-depth regression in oauth.ts remains unaddressed. Consider the mitigations suggested in the previous review (configurable allowlist, documentation, or explicit risk acceptance) before shipping to production.
Discarded (0)
| Location | Issue | Reason Discarded |
|---|---|---|
| — | — | No findings were discarded |
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: No reviewers dispatched — delta contained only a resolved changeset message fix with no new substantive code to review.
1 participant
No description provided.