fix: parse OIDC account linking flag as boolean - #717
Conversation
Parse OIDC_ALLOW_DANGEROUS_EMAIL_LINKING as a JSON boolean so "false" does not enable dangerous account linking.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesOIDC email linking configuration
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/env.ts`:
- Around line 142-144: Update OIDC_ALLOW_DANGEROUS_EMAIL_LINKING to pass the
parsed JSON value directly instead of wrapping it with Boolean(...), allowing
the existing z.boolean() validation to reject non-boolean configuration values
while preserving the false default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| OIDC_ALLOW_DANGEROUS_EMAIL_LINKING: Boolean( | ||
| JSON.parse(process.env.OIDC_ALLOW_DANGEROUS_EMAIL_LINKING || 'false'), | ||
| ), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require an exact JSON boolean before enabling account linking.
Boolean(JSON.parse(...)) converts non-boolean JSON values to booleans. For example, 1, the JSON string "false", and {} become true. This can enable allowDangerousEmailAccountLinking in src/server/auth.ts with an invalid configuration value.
Parse the JSON value without Boolean(...) and let z.boolean() reject non-boolean values.
Proposed fix
- OIDC_ALLOW_DANGEROUS_EMAIL_LINKING: Boolean(
- JSON.parse(process.env.OIDC_ALLOW_DANGEROUS_EMAIL_LINKING || 'false'),
- ),
+ OIDC_ALLOW_DANGEROUS_EMAIL_LINKING: JSON.parse(
+ process.env.OIDC_ALLOW_DANGEROUS_EMAIL_LINKING || 'false',
+ ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| OIDC_ALLOW_DANGEROUS_EMAIL_LINKING: Boolean( | |
| JSON.parse(process.env.OIDC_ALLOW_DANGEROUS_EMAIL_LINKING || 'false'), | |
| ), | |
| OIDC_ALLOW_DANGEROUS_EMAIL_LINKING: JSON.parse( | |
| process.env.OIDC_ALLOW_DANGEROUS_EMAIL_LINKING || 'false', | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/env.ts` around lines 142 - 144, Update OIDC_ALLOW_DANGEROUS_EMAIL_LINKING
to pass the parsed JSON value directly instead of wrapping it with Boolean(...),
allowing the existing z.boolean() validation to reject non-boolean configuration
values while preserving the false default.
|
Boolean handling is currently inconsistent and confusing with the JSON.parse wrapping and certain values can unexpectedly resolve to true. What would you think about a PR that updated all of the env boolean parsing logic to call a function such as: const parseEnvBoolean = (value: string | undefined): boolean => {
return '1' === value || 'true' === value?.toLowerCase();
}; |
|
Certainly! That was an oversight on my part, thanks for cleaning it up :) |
Description
Parse OIDC_ALLOW_DANGEROUS_EMAIL_LINKING as a JSON boolean so "false" does not enable dangerous account linking.
Checklist
CONTRIBUTING.mdin its entiretySummary by CodeRabbit