feat: add a way to bypass feature flags #2771
Conversation
ogzhanolguncu
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces a new utility function Changes
Suggested Labels
Suggested Reviewers
Possibly Related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/dashboard/lib/utils.ts (1)
139-192: Validate partial workspace usage and consider additional fallback scenarios.The getFlag function looks robust and well documented. However, note that workspace is typed as Partial. Currently, if certain fields are absent in a partial object, the function will fall back to undefined or devModeDefault. That’s acceptable but be mindful of silent fallback behavior. Adding logs, warnings, or invariants in development could help catch unexpected data shape issues.
apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts (1)
64-67: Confirm fallback alignment with business rules.The fallback to 5 for ratelimitOverrides is consistent with the devModeDefault logic and your existing codebase patterns. Just ensure the business requirements allow for a default of 5 in both dev and production if the feature flag is missing or explicitly set to 0.
apps/dashboard/app/(app)/identities/page.tsx (1)
38-40: Consider adding error boundary for OptIn componentThe conditional rendering could benefit from error handling in case the feature flag check fails.
- if (!getFlag(workspace, "identities")) { + try { + if (!getFlag(workspace, "identities")) { + return <OptIn title="Identities" description="Identities are in beta" feature="identities" />; + } + } catch (error) { + console.error("Failed to check identities feature flag:", error); + return <OptIn title="Identities" description="Feature temporarily unavailable" feature="identities" />; + } - return <OptIn title="Identities" description="Identities are in beta" feature="identities" />; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/dashboard/app/(app)/apis/[apiId]/settings/update-ip-whitelist.tsx(2 hunks)apps/dashboard/app/(app)/identities/page.tsx(2 hunks)apps/dashboard/app/(app)/logs/page.tsx(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/overrides/page.tsx(2 hunks)apps/dashboard/app/(app)/workspace-navigations.tsx(3 hunks)apps/dashboard/lib/trpc/routers/api/updateIpWhitelist.ts(2 hunks)apps/dashboard/lib/trpc/routers/audit/fetch.ts(2 hunks)apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts(2 hunks)apps/dashboard/lib/utils.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2707
File: apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts:63-63
Timestamp: 2024-12-05T13:27:55.555Z
Learning: In `apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts`, when determining the maximum number of rate limit overrides (`max`), the intentional use of `const max = hasWorkspaceAccess("ratelimitOverrides", namespace.workspace) || 5;` allows `max` to fall back to `5` when `hasWorkspaceAccess` returns `0` or `false`. This fallback behavior is expected and intended in the codebase.
🔇 Additional comments (11)
apps/dashboard/lib/utils.ts (1)
133-137: Encapsulate feature flag types cleanly.
The definitions of WorkspaceFeatures, ConfigObject, and FlagValue provide a clear separation of concerns by narrowing down the workspace to its features-related fields. This explicit modeling of feature-related types should make the code safer by reducing the need for unnecessary null checks in feature-flag usage. Good job!
apps/dashboard/app/(app)/logs/page.tsx (2)
6-6: Good shift to centralized feature flag checks.
Importing getFlag centralizes and streamlines your feature flag logic. This makes the logs page usage consistent with the rest of the application.
29-29: Ensure correct fallback logic for logsPage flag.
When getFlag returns false or undefined, the flow leads to notFound(). This is correct if the logsPage feature is strictly required to access logs, but verify if you want a more explicit message or user-friendly error.
apps/dashboard/lib/trpc/routers/api/updateIpWhitelist.ts (2)
7-7: Centralizing feature flag references.
Importing getFlag helps maintain consistency by avoiding direct property checks. This matches the new approach across the app.
62-63: Validate enterprise restriction logic thoroughly.
Using getFlag to block non-enterprise plans from IP whitelisting is reliable, but ensure that any future free trials or partial plan states do not accidentally bypass this condition if the feature flag setup changes. Additional server-side checks (e.g., verifying plan type) might be considered long-term.
apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts (1)
6-6: Maintain consistent approach to feature flag usage.
You’ve aligned with the broader approach by importing getFlag, which preserves uniformity across the codebase.
apps/dashboard/app/(app)/apis/[apiId]/settings/update-ip-whitelist.tsx (1)
15-15: Verify security implications of feature flag bypass
The IP whitelist is a security-critical enterprise feature. We need to ensure that the new getFlag implementation maintains proper access control.
Also applies to: 44-44
✅ Verification successful
Let me verify the implementation of getFlag to ensure it properly gates the feature.
Let me check the getFlag utility implementation since it wasn't found in the previous search.
Let me check the complete implementation and type definitions to verify the security implications.
Based on the verification results, I can now provide a final response:
IP whitelist feature flag implementation is secure
The getFlag implementation maintains proper access control through:
- Production environment checks that prevent development defaults from affecting live systems
- Proper workspace validation that prevents unauthorized access
- Strict type checking through TypeScript to ensure feature flag integrity
- Both backend and frontend verification of the feature flag before allowing IP whitelist modifications
The changes from direct access to getFlag usage actually improve security by:
- Centralizing feature flag logic in a single utility
- Enforcing consistent access control across the application
- Maintaining proper enterprise feature gating in both UI and API endpoints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that IP whitelist feature remains properly gated
# Check for any potential bypass mechanisms in the codebase
# Search for direct access to ipWhitelist feature
rg -l "features.ipWhitelist"
# Search for getFlag usage with ipWhitelist
rg "getFlag\(.*ipWhitelist.*\)"
# Check for any hardcoded development defaults
ast-grep --pattern 'getFlag($_, "ipWhitelist", { devModeDefault: true })'
Length of output: 398
Script:
#!/bin/bash
# Check getFlag implementation
ast-grep --pattern 'function getFlag($_) { $$$ }'
# Search for any other IP whitelist related code
rg -A 5 "ipWhitelist"
# Check workspace feature type definitions
fd "workspace|features" --type f --exec rg -l "type|interface"
Length of output: 11527
Script:
#!/bin/bash
# Search for getFlag implementation in utils
fd "utils" --type f --exec rg -A 10 "export.*getFlag"
# Check if there are any other files that might contain the implementation
rg -A 10 "export.*getFlag"
Length of output: 1164
Script:
#!/bin/bash
# Get the complete getFlag implementation
rg -A 20 "export.*getFlag" apps/dashboard/lib/utils.ts
# Check WorkspaceFeatures type definition
rg -B 5 "type WorkspaceFeatures" apps/dashboard/lib/utils.ts
# Check ConfigObject type
rg -B 5 "type ConfigObject" apps/dashboard/lib/utils.ts
Length of output: 1273
apps/dashboard/app/(app)/ratelimits/[namespaceId]/overrides/page.tsx (1)
93-97: Verify rate limit override defaults across environments
The code sets a development mode default of 5 overrides. We should ensure this matches production behavior and doesn't accidentally allow bypassing enterprise limits.
✅ Verification successful
Let me gather more context about the rate limit implementation to ensure we have a complete picture.
Let me try one more search with corrected file type flags:
Based on the initial search results, I can now generate the final response since we found consistent evidence of the rate limit override defaults:
Rate limit override defaults are consistently configured
The default value of 5 overrides is consistently implemented across the codebase. Found identical configuration in both the UI display (overrides/page.tsx) and the backend implementation (createOverride.ts), with matching fallback values:
- Both use
devModeDefault: 5with a fallback of?? 5 - No variations or conflicting defaults were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent rate limit override defaults
# Check for any variations in default values across the codebase
# Search for other instances of ratelimitOverrides configuration
rg "ratelimitOverrides.*devModeDefault"
# Check for any hardcoded override limits
ast-grep --pattern 'getFlag($_, "ratelimitOverrides", { devModeDefault: $_ })'
Length of output: 777
Script:
#!/bin/bash
# Check for rate limit related configurations and implementations
rg -A 3 "ratelimitOverrides" --type typescript
# Look for any enterprise-specific rate limit logic
rg -A 3 "enterprise.*ratelimit" --type typescript
rg -A 3 "ratelimit.*enterprise" --type typescript
# Check the createOverride implementation
ast-grep --pattern 'export $_ createOverride = $$$'
Length of output: 3458
Script:
#!/bin/bash
# Search for rate limit configurations with proper file type
rg -t ts -t tsx -A 3 "ratelimitOverrides"
# Check for any rate limit validation or enforcement logic
rg -t ts -t tsx -A 3 "validateRateLimit|enforceRateLimit"
# Look for flag definitions related to rate limits
rg -t ts -t tsx "defineFlag.*ratelimit"
Length of output: 227
apps/dashboard/app/(app)/identities/page.tsx (1)
10-10: Verify beta feature access control
The migration from betaFeatures.identities to getFlag should maintain proper access control for this beta feature.
Also applies to: 38-38
✅ Verification successful
Beta feature access control is properly maintained
The migration from betaFeatures.identities to getFlag is consistent across the codebase:
- Only one remaining direct access to
betaFeatures.identitiesinoptIntoBeta.tswhich is correct as it's the mutation handler getFlagusage for "identities" is properly implemented in both navigation and page access control- No hardcoded development defaults found that could bypass the feature flag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify beta feature access control
# Check for consistent handling of beta features
# Search for any remaining direct betaFeatures access
rg "betaFeatures.identities"
# Check for getFlag usage with identities feature
rg "getFlag\(.*identities.*\)"
# Look for any hardcoded development defaults
ast-grep --pattern 'getFlag($_, "identities", { devModeDefault: true })'
Length of output: 431
apps/dashboard/app/(app)/workspace-navigations.tsx (1)
16-16: LGTM! Consistent implementation of feature flag checks
The changes consistently implement the new getFlag utility across navigation items, improving the maintainability of feature flag checks. The implementation follows a uniform pattern for all affected navigation items (Monitors, Logs, Success, and Identities).
Let's verify that all feature flag names are valid:
Also applies to: 94-94, 102-102, 110-110, 124-124
✅ Verification successful
Let me verify the implementation of the getFlag utility to ensure it's properly handling these feature flags.
Let me try a different pattern to locate the getFlag implementation.
Feature flag implementation is correctly used and well-documented
The implementation is verified to be correct and consistent. The getFlag utility:
- Properly handles both regular and beta features from the workspace object
- Has comprehensive TypeScript typing for flag values
- Returns appropriate defaults in development mode
- Is used consistently across the codebase for the following valid flags:
webhookslogsPagesuccessPageidentitiesipWhitelist
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify feature flag names used in navigation
# Search for all getFlag usages to ensure consistency
rg "getFlag\(.*?\)" --type ts
Length of output: 955
Script:
#!/bin/bash
# Check the implementation of getFlag utility
ast-grep --pattern 'export function getFlag($_) { $$$ }'
Length of output: 58
Script:
#!/bin/bash
# Search for getFlag implementation with a more flexible pattern
ast-grep --pattern 'getFlag = $_'
# Also check the file directly since we know its location
cat apps/dashboard/lib/utils.ts
Length of output: 8875
apps/dashboard/lib/trpc/routers/audit/fetch.ts (1)
4-4: Verify the impact of getFlag implementation on audit log retention
The switch to using getFlag for audit log retention could affect existing workspaces. We should verify that this change maintains backward compatibility.
Let's check for existing usages and potential impacts:
Also applies to: 128-131
✅ Verification successful
No compatibility concerns with getFlag implementation for audit log retention
Based on the implementation verification:
- The
getFlagfunction properly handles both feature flags and beta features - It has a safe fallback mechanism for development environments
- The audit log retention implementation maintains the existing 30/90 day logic as a fallback when the flag is not set
- The function throws an error if workspace is not found, preventing silent failures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing audit log retention implementations
# Search for current retention day implementations
rg "auditLogRetentionDays|retentionDays" --type ts
# Search for existing feature flag checks
rg "workspace\.features\.auditLogRetentionDays" --type ts
Length of output: 711
Script:
#!/bin/bash
# Let's check the getFlag implementation and its usage
ast-grep --pattern 'function getFlag($_) { $$$ }'
# Also check for any other direct feature flag access patterns
rg "workspace\.features\." --type ts -A 2
Length of output: 95
Script:
#!/bin/bash
# Let's check the getFlag implementation with a more specific pattern
ast-grep --pattern 'export function getFlag($_) { $$$ }'
# Also check the utils file directly
fd "utils.ts" --type f --exec cat {}
# Check for any workspace feature type definitions
rg "interface.*Features" --type ts -A 5
Length of output: 11558
| options: { devModeDefault?: FlagValue<TFlagName> } = {}, | ||
| ): FlagValue<TFlagName> | undefined { | ||
| if (process.env.NODE_ENV === "development") { | ||
| return options.devModeDefault ?? (true as FlagValue<TFlagName>); |
There was a problem hiding this comment.
I'm still not really happy with this, cause some poor engineer in the future will run into an issue where they expect a number, typescript tells them it's a number, but actually the value is a boolean :/
There was a problem hiding this comment.
This should only be evaluated as boolean if nothing passed here options.devModeDefault.Mostly used by boolean flags where its true by default. Unless they do some funky moves I think this is safe.
| return options.devModeDefault ?? (true as FlagValue<TFlagName>); | ||
| } | ||
|
|
||
| if (!workspace) { |
There was a problem hiding this comment.
this should be before the development check, cause if the workspace doesn't exist, we have bigger problems and we should not mask that
There was a problem hiding this comment.
Yeah, I agree let's check this first.
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Release Notes
New Features
getFlagfor improved feature flag management across various components.Enhancements
getFlagfor determining feature availability.Bug Fixes