Closed
Conversation
## Security Fixes - Fix authorization check in CoursesController.Update() to prevent unauthorized course modifications - Replace AllowAllOrigins CORS policy with restrictive environment-based configuration - Add HTTPS enforcement middleware for production environments - Remove duplicate FluentValidation package reference ## Changes **src/Learnify.Api/Controllers/CoursesController.cs** - Add ownership verification before allowing course updates - Only course owners or admins can modify courses - Matches authorization pattern used in Delete() method **src/Learnify.Api/Program.cs** - Create DevelopmentCors policy (allows all for local testing) - Create ProductionCors policy (restricted to allowed origins) - Add HTTPS redirection and HSTS in production - Switch CORS policy based on environment **src/Learnify.Api/appsettings.json** - Add AllowedOrigins configuration section - Default origins: localhost:3000, localhost:5173, https://yourdomain.com **src/Learnify.Api/appsettings.Development.json** (new) - Development-specific configuration - Allows localhost origins for local development **src/Learnify.Application/Learnify.Application.csproj** - Remove duplicate FluentValidation package reference ## Impact - 🔒 Enhanced security: Users cannot modify courses they don't own - 🔒 Production-ready CORS: No more AllowAllOrigins vulnerability - 🔒 HTTPS enforcement: All production traffic forced to HTTPS - ✨ Clean build: No duplicate package conflicts ## Testing - Test course update with non-owner user → Should return 403 Forbidden - Test CORS in production → Only allowed origins accepted - Test HTTP in production → Should redirect to HTTPS Closes #Phase1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 1: Critical Security Fixes 🔒
Overview
This PR addresses critical security vulnerabilities identified in the Learnify PRD Phase 1. All changes are focused on enhancing security and production-readiness.
📋 Changes Summary
CoursesController.csProgram.csappsettings.jsonappsettings.Development.jsonLearnify.Application.csproj🔒 Security Improvements
1. Authorization Fix in CoursesController.Update()
Before:
After:
Impact: Users can no longer modify courses they don't own.
2. Restrictive CORS Policy
Added:
ProductionCors- Restricts to allowed origins from configurationDevelopmentCors- Allows all origins for local testingConfiguration (appsettings.json):
Impact: Eliminates
AllowAnyOrigin()security vulnerability.3. HTTPS Enforcement
Added:
Impact: All production traffic is forced to HTTPS.
4. Clean Build
Removed: Duplicate FluentValidation package reference.
Impact: Eliminates potential build conflicts.
🧪 Testing Checklist
AllowedOriginsaccepted📊 Risk Assessment
🔄 Backward Compatibility
Breaking Changes: None
Configuration Required: Yes - Add
AllowedOriginstoappsettings.jsonfor production.📝 Related Issues
✨ Next Steps (After Merge)
Reviewer: Please verify: