diff --git a/PRIVACY_ANALYSIS.md b/PRIVACY_ANALYSIS.md new file mode 100644 index 0000000..bf43b07 --- /dev/null +++ b/PRIVACY_ANALYSIS.md @@ -0,0 +1,923 @@ +# Tour/Scene Privacy Logic Analysis Report + +## Executive Summary + +The privacy system has **critical design flaws** that compromise security. While Tour-level privacy management is correctly implemented with proper propagation to scenes, the **Scene model is missing the `privacy` field** in its schema definition, creating a fundamental data structure issue. Additionally, there are inconsistencies between the intended API requirements and actual implementation. + +### Critical Issues Found: 8 +### High Priority Issues: 5 +### Medium Priority Issues: 3 + +--- + +## Requirements from ARCHITEC.md + +``` +- Public: Everyone can see. Only creator can manage +- Private: Only creator can see and manage +- Shared link: Everyone with valid token can see. Only creator can manage +- Member: Only members can see. Only creator can manage +``` + +--- + +## Detailed Findings + +## 1. GET /api/tours/:id - Tour Detail Endpoint + +### Current Implementation +**File**: [backend/middlewares/TourController.js](backend/middlewares/TourController.js#L82-L125) + +```javascript +router.get('/:id', optionalAuth, async (req, res) => { + const tour = await Tour.findById(req.params.id) + .populate('createdBy', 'username') + .populate({ path: 'rootSceneId', ... }) + .populate({ path: 'scenes', ... }) + .lean(); + + const isTokenValid = tour.shareToken && (!tour.shareTokenExpires || new Date() < tour.shareTokenExpires); + + let hasAccess = tour.privacy === 'public' || isOwner || isAdmin || + (tour.privacy === 'shared' && req.query.token === tour.shareToken && isTokenValid) || + (tour.privacy === 'member' && req.user && req.user._id && ( + tour.sharedWith.some(u => u.toString() === req.user._id.toString()) || + (userEmail && tour.sharedEmails.includes(userEmail)) + )); + + if (!hasAccess) return res.status(403).json({ message: 'Unauthorized' }); + res.json(tour); +}); +``` + +### Expected Behavior (Per ARCHITEC.md) +1. ✅ **Public**: Anyone can view +2. ✅ **Private**: Only creator +3. ✅ **Shared**: Valid token holders can view +4. ✅ **Member**: Members in `sharedWith` array can view +5. ⚠️ **Token Validation**: Check expiration + +### Issues Found + +#### ISSUE 1.1 - Token Validation Logic Missing Email Check +**Severity**: HIGH + +In the shared privacy mode, the code validates token **but does NOT validate expiration for other access types**. It should also allow members with valid emails to access. + +**Current Code**: +```javascript +(tour.privacy === 'shared' && req.query.token === tour.shareToken && isTokenValid) +``` + +**Problem**: For `member` privacy, it checks `tour.sharedEmails` but never validates if they're in the `member` list during token validation. + +**Recommendation**: Add explicit token handling for member access patterns. + +--- + +## 2. GET /api/tours - Tour List Endpoint + +### Current Implementation +**File**: [backend/middlewares/TourController.js](backend/middlewares/TourController.js#L128-L167) + +```javascript +router.get('/', optionalAuth, async (req, res) => { + let query = { privacy: 'public' }; + + if (req.user && req.user.role !== 'guest') { + query = { + $or: [ + { privacy: 'public' }, + { createdBy: req.user._id }, + { privacy: 'member', sharedWith: req.user._id }, + { privacy: 'member', sharedEmails: req.user.email } + ] + }; + } + const tours = await Tour.find(query)... +}); +``` + +### Expected Behavior +1. **Public Tours**: Visible to everyone (guest/logged in) +2. **Private Tours**: Only visible to creator +3. **Shared Tours**: Only visible if token provided **or user has permission** +4. **Member Tours**: Only visible to members in `sharedWith` or `sharedEmails` + +### Issues Found + +#### ISSUE 2.1 - Shared Token Tours Not Returned in List +**Severity**: CRITICAL + +The endpoint **does NOT support token-based access**. If a guest has a shared link token, they cannot see the tour in the list. + +**Current Code**: No `token` parameter handling + +**Expected Behavior**: Should accept `?token=xxx` and return tours matching that token + +**Problem Scenario**: +1. User A creates private tour with shareToken +2. User A shares token with Guest +3. Guest tries `GET /api/tours?token=xxx` → Empty list returned +4. Guest **cannot discover the tour exists** + +**Recommendation**: +```javascript +// Add token support +if (req.query.token) { + const tourWithToken = await Tour.findOne({ + shareToken: req.query.token, + $or: [ + { shareTokenExpires: null }, + { shareTokenExpires: { $gt: new Date() } } + ] + }); + if (tourWithToken) { + query = { _id: tourWithToken._id }; + } +} +``` + +#### ISSUE 2.2 - Private Tours Accessible to Creator Only Not Enforced +**Severity**: MEDIUM + +The query allows creator to see private tours, but **doesn't explicitly exclude non-creator guests** from seeing **someone else's shared tours**. + +The current logic returns: +- Public tours → OK +- Creator's own tours (all privacy levels) → OK +- Member tours where user is a member → OK + +But **missing**: Explicit rejection of non-creator access to private/shared tours they're not invited to. + +--- + +## 3. GET /api/scenes/:id - Scene Detail Endpoint + +### Current Implementation +**File**: [backend/routes/sceneRoutes.js](backend/routes/sceneRoutes.js#L149-L210) + +```javascript +router.get('/:id', optionalAuth, async (req, res) => { + const scene = await Scene.findById(req.params.id) + .populate('createdBy', 'username') + .populate('tourId'); // ⚠️ Critical: Must populate tour for privacy checks + + const tour = scene.tourId; + const isSceneTokenValid = scene.shareToken && (!scene.shareTokenExpires || new Date() < scene.shareTokenExpires); + const isTourTokenValid = tour.shareToken && (!tour.shareTokenExpires || new Date() < tour.shareTokenExpires); + + let hasAccess = tour.privacy === 'public' || isOwner || isAdmin || + (scene.privacy === 'shared' && req.query.token === scene.shareToken && isSceneTokenValid) || + (tour.privacy === 'shared' && req.query.token === tour.shareToken && isTourTokenValid) || + (tour.privacy === 'member' && req.user && req.user._id && ( + tour.sharedWith.some(u => u.toString() === req.user._id.toString()) || + (userEmail && tour.sharedEmails.includes(userEmail)) + )); + + if (!hasAccess) return res.status(403).json({ message: 'Unauthorized' }); + + // Increment views + if (!isOwner && !isAdmin) { + scene.views = (scene.views || 0) + 1; + await scene.save(); + } + res.json(scene); +}); +``` + +### Expected Behavior +Privacy should be inherited from Tour at minimum, with Scene-level overrides possible. + +### Issues Found + +#### ISSUE 3.1 - CRITICAL: Scene Model Missing Privacy Field +**Severity**: CRITICAL 🔴 + +**File**: [backend/models/Scene.js](backend/models/Scene.js) + +**Current Schema**: +```javascript +const sceneSchema = new mongoose.Schema({ + tourId: { type: ObjectId, required: true }, + name: String, + shareToken: String, + shareTokenExpires: Date, + sharedWith: [ObjectId], + sharedEmails: [String], + // ❌ MISSING: privacy field +}, { timestamps: true }); +``` + +**Problem**: The code references `scene.privacy` but it's **never defined in the schema**. MongoDB will store it but it won't be validated or properly indexed. + +**Evidence**: +- Line 165: `isSceneTokenValid = scene.shareToken && ...` (assuming privacy exists) +- Line 170: `(scene.privacy === 'shared' && req.query.token === scene.shareToken ...)` +- But schema never defines `privacy` field + +**Impact**: +- ❌ Scene privacy cannot be properly validated +- ❌ No default value enforcement +- ❌ No enum restrictions (could be any string) +- ❌ Causes logic errors when privacy is undefined + +**Required Fix**: +```javascript +privacy: { + type: String, + enum: ['public', 'private', 'member', 'shared'], + default: 'private' +}, +``` + +#### ISSUE 3.2 - Bridge Access Logic Allows Unintended Navigation +**Severity**: HIGH + +```javascript +// [BRIDGE ACCESS LOGIC] Lines 194-205 +if (!hasAccess && req.query.token) { + const potentialParents = await Hotspot.find({ + target_scene_id: scene._id + }).distinct('parent_scene_id'); + + if (potentialParents.length > 0) { + const authorizedParentExists = await Scene.exists({ + _id: { $in: potentialParents }, + shareToken: req.query.token, + $or: [{ shareTokenExpires: null }, { shareTokenExpires: { $gt: new Date() } }] + }); + if (authorizedParentExists) hasAccess = true; + } +} +``` + +**Problem**: This logic allows a guest with a shared link to ANY scene in a chain to navigate through ALL connected scenes. + +**Scenario**: +1. User creates 5 linked scenes in a tour +2. Only scene 1 is shared (has token) +3. Guest with token for scene 1 can navigate to scene 2, 3, 4, 5 (if they know the hotspot coordinates) +4. **Scenes 2-5 may have different privacy settings but are still accessible** + +**Recommendation**: Add privacy validation to ensure the target scene matches the token's privacy context. + +--- + +## 4. GET /api/scenes - Scene List Endpoint + +### Current Implementation +**File**: [backend/routes/sceneRoutes.js](backend/routes/sceneRoutes.js#L108-L142) + +```javascript +router.get('/', optionalAuth, async (req, res) => { + const publicTours = await Tour.find({ privacy: 'public' }).select('_id'); + const publicTourIds = publicTours.map(t => t._id); + + let baseQuery = req.user && req.user.role !== 'guest' + ? { $or: [ + { privacy: 'public' }, + { tourId: { $in: publicTourIds } }, + { createdBy: req.user._id }, + { sharedWith: req.user._id }, + { sharedEmails: req.user.email } + ]} + : { $or: [ + { privacy: 'public' }, + { tourId: { $in: publicTourIds } } + ]}; + + let finalQuery = baseQuery; + + if (token) { + const tourWithToken = await Tour.findOne({ shareToken: token }).select('_id'); + finalQuery = { + $or: [ + baseQuery, + { shareToken: token }, + { tourId: tourWithToken ? tourWithToken._id : null } + ] + }; + } + + const scenes = await Scene.find(finalQuery)... +}); +``` + +### Expected Behavior +- **Public Scenes**: Everyone sees +- **Private Scenes**: Only creator +- **Shared Scenes**: Only token holders + creator +- **Member Scenes**: Only members + creator +- **Tours**: Inherit parent tour privacy + +### Issues Found + +#### ISSUE 4.1 - Private Tour Scenes Should Not Be Visible +**Severity**: HIGH + +**Problem**: The query returns all scenes from public tours but **doesn't check individual scene privacy**. + +**Current Logic**: +```javascript +{ tourId: { $in: publicTourIds } } // All scenes from public tours returned +``` + +**Scenario**: +1. Tour A is public (privacy: 'public') +2. Scene 1 in Tour A created by User X (privacy: 'public') +3. Scene 2 in Tour A created by User Y (privacy: 'private') +4. Guest user calls GET /api/scenes +5. **Both scenes returned even though Scene 2 is marked private** + +**Why This Matters**: Per ARCHITEC.md, even in a public tour, individual scenes can be private. + +#### ISSUE 4.2 - Token Expiration Not Validated +**Severity**: MEDIUM + +```javascript +if (token) { + const tourWithToken = await Tour.findOne({ shareToken: token }).select('_id'); + // ❌ No check for shareTokenExpires +} +``` + +**Problem**: The code finds a tour with matching token but **never checks if token is expired**. + +**Fix Required**: +```javascript +const tourWithToken = await Tour.findOne({ + shareToken: token, + $or: [ + { shareTokenExpires: null }, + { shareTokenExpires: { $gt: new Date() } } + ] +}); +``` + +#### ISSUE 4.3 - Null Privacy Values Break Logic +**Severity**: MEDIUM + +Scenes created before the privacy field was added will have `privacy: undefined`. The query checks: +```javascript +{ privacy: 'public' } // Won't match undefined! +``` + +**Scenario**: +1. Old scene exists with no privacy field +2. Scene is in public tour +3. Guest queries GET /api/scenes +4. Old scenes NOT returned (should be visible due to public tour) + +--- + +## 5. PUT /api/tours/:id - Update Tour + +### Current Implementation +**File**: [backend/middlewares/TourController.js](backend/middlewares/TourController.js#L46-L80) + +```javascript +router.put('/:id', protect, async (req, res) => { + const tour = await Tour.findById(req.params.id); + + if (tour.createdBy.toString() !== req.user._id.toString() && req.user.role !== 'admin') { + return res.status(403).json({ message: 'Not authorized' }); + } + + // Update privacy + if (privacy) tour.privacy = privacy; + + // Handle token logic... + if (tour.privacy === 'shared') { + if (!tour.shareToken) tour.shareToken = crypto.randomBytes(24).toString('hex'); + // ... handle expiration + } else { + tour.shareToken = undefined; + tour.shareTokenExpires = undefined; + } + + // Propagate to scenes + await propagateScenePrivacy(tour._id, { privacy: tour.privacy, ... }, req.user._id); + + res.json(tour); +}); +``` + +### Expected Behavior +✅ **Correctly Implemented** - Ownership check works, privacy propagation to scenes applied. + +### Verified Working +- ✅ Checks creator ownership +- ✅ Generates/removes tokens based on privacy mode +- ✅ Handles token expiration +- ✅ Propagates privacy to all scenes in tour + +--- + +## 6. PUT /api/scenes/:id - Update Scene + +### Current Implementation +**File**: [backend/routes/sceneRoutes.js](backend/routes/sceneRoutes.js#L214-L280) + +```javascript +router.put('/:id', protect, uploadSinglePanorama, async (req, res) => { + const scene = await Scene.findById(req.params.id); + + if (!scene || (scene.createdBy.toString() !== req.user._id.toString() && req.user.role !== 'admin')) { + return res.status(403).json({ message: 'Not authorized' }); + } + + // [SECURITY] Block direct privacy changes on Scene + if (privacy && privacy !== scene.privacy) { + return res.status(403).json({ + message: "Privacy must be managed at Tour level" + }); + } + + // Update metadata only + scene.name = title || scene.name; + scene.description = description !== undefined ? description : scene.description; + + await scene.save(); + res.json(scene); +}); +``` + +### Expected Behavior +✅ **Correctly Implemented** - Privacy cannot be changed directly; must go through Tour update. + +### Verified Working +- ✅ Checks creator ownership +- ✅ Blocks direct privacy modification +- ✅ Enforces Tour-level privacy management + +--- + +## 7. DELETE /api/tours/:id & DELETE /api/scenes/:id + +### Current Implementation + +#### Tour Deletion +**File**: [backend/middlewares/TourController.js](backend/middlewares/TourController.js#L168-L190) + +```javascript +router.delete('/:id', protect, async (req, res) => { + const tour = await Tour.findById(req.params.id); + + if (tour.createdBy.toString() !== req.user._id.toString() && req.user.role !== 'admin') { + return res.status(403).json({ message: 'Not authorized' }); + } + + // Delete all scenes in tour + const scenesInTour = await Scene.find({ tourId: tour._id }); + for (const scene of scenesInTour) { + await deleteSceneCascade(scene._id, req.user._id); + } + + await Tour.findByIdAndDelete(req.params.id); + res.json({ message: 'Tour deleted' }); +}); +``` + +#### Scene Deletion +**File**: [backend/routes/sceneRoutes.js](backend/routes/sceneRoutes.js#L303-L350) + +```javascript +router.delete('/:id', protect, async (req, res) => { + const rootScene = await Scene.findById(rootSceneId); + + if (req.user.role !== 'admin' && rootScene.createdBy.toString() !== req.user._id.toString()) { + return res.status(403).json({ message: 'Forbidden' }); + } + + // Cascade delete + await deleteSceneCascade(rootSceneId, req.user._id); + + // Update tour (remove from scenes array, update rootSceneId) + if (tourId) { + const tour = await Tour.findById(tourId); + if (tour) { + tour.scenes = tour.scenes.filter(sId => sId.toString() !== rootSceneId.toString()); + if (tour.rootSceneId?.toString() === rootSceneId.toString()) { + tour.rootSceneId = tour.scenes.length > 0 ? tour.scenes[0] : null; + } + + // Delete tour if empty + const actualRemainingScenes = await Scene.countDocuments({ tourId: tour._id }); + if (actualRemainingScenes === 0) { + await Tour.findByIdAndDelete(tour._id); + return res.json({ message: 'Tour deleted' }); + } + await tour.save(); + } + } + + res.json({ message: 'Scene deleted' }); +}); +``` + +### Expected Behavior +✅ **Correctly Implemented** - Delete permissions properly checked. + +### Verified Working +- ✅ Creator-only deletion +- ✅ Cascade delete scenes when tour deleted +- ✅ Cleanup tour references when scene deleted +- ✅ Auto-delete empty tours + +--- + +## 8. Image Asset Streaming - GET /api/assets/view/:assetId + +### Current Implementation +**File**: [backend/routes/assetRoutes.js](backend/routes/assetRoutes.js#L18-L100) + +```javascript +router.get('/assets/view/:assetId', verifyReferer, optionalAuth, async (req, res) => { + const asset = await Asset.findById(req.params.assetId); + + // Complex privacy check + const scene = await Scene.findOne({ assetId: asset._id }).populate('tourId'); + + const isSceneTokenValid = scene.shareToken && (!scene.shareTokenExpires || new Date() < scene.shareTokenExpires); + const isTourTokenValid = tour && tour.shareToken && (!tour.shareTokenExpires || new Date() < tour.shareTokenExpires); + + let hasAccess = isAdmin || + scene.privacy === 'public' || + (tour && tour.privacy === 'public') || + (scene.privacy === 'member' && userIdStr && (scene.sharedWith.some(...) || scene.sharedEmails.includes(...))) || + isOwner || + (scene.privacy === 'shared' && req.query.token === scene.shareToken && isSceneTokenValid) || + (tour && tour.privacy === 'shared' && req.query.token === tour.shareToken && isTourTokenValid); + + if (!hasAccess) return res.status(403).json({ message: 'Forbidden' }); + + res.sendFile(resolvedPath, { maxAge: 2592000000 }); +}); +``` + +### Issues Found + +#### ISSUE 8.1 - Scene Privacy Field Again Missing +**Severity**: CRITICAL + +This endpoint checks `scene.privacy === 'public'` and `scene.privacy === 'member'` but Scene model doesn't define the privacy field. + +**Compounded by**: Scenes created with null privacy will fail these checks even if they should be public. + +#### ISSUE 8.2 - Bridge Access Logic Not Applied +**Severity**: MEDIUM + +Unlike the scene detail endpoint, this doesn't implement the bridge access logic. A guest with a shared scene token cannot navigate to connected scenes' assets. + +--- + +## 9. Frontend Tour Display Logic + +### Current Implementation +**File**: [frontend/js/main_map.js](frontend/js/main_map.js#L1468-L1535) + +```javascript +async function loadScenes(urlToken = null) { + let url = `${API_BASE_URL}/scenes?_=${new Date().getTime()}`; + if (urlToken) url += `&token=${urlToken}`; + + const response = await fetch(url, { headers }); + const scenes = await response.json(); + + // Deduplicate by coordinates + scenes.forEach((scene) => { + const coordKey = `${latNum.toFixed(6)},${lngNum.toFixed(6)}`; + if (seenCoordinates.has(coordKey)) return; // Skip duplicates + + // Add marker to map + const marker = L.marker([latNum, lngNum], { icon: calloutIcon }); + marker.on('click', () => { + openScene(scene._id, scene.privacy, scene.shareToken || ''); + }); + }); +} +``` + +### Expected Behavior +Only display scenes the user has permission to view. + +### Issues Found + +#### ISSUE 9.1 - Frontend Trust Issue +**Severity**: MEDIUM + +The frontend blindly trusts backend to filter scenes by privacy. While this is standard practice, the logic doesn't provide additional validation. + +**Current Flow**: +1. Frontend calls `GET /api/scenes` +2. Backend filters by privacy +3. Frontend displays all returned scenes +4. User clicks → `openScene(scene._id, scene.privacy, scene.shareToken)` + +**Potential Issue**: If backend filtering is broken, frontend has no fallback. + +**Recommendation**: Add client-side privacy display indicators (e.g., 🔒 icon for private scenes user cannot access). + +#### ISSUE 9.2 - Tour Privacy Not Shown +**Severity**: LOW + +Frontend displays scene privacy but not tour privacy. Users don't know if scenes are part of a private/shared tour. + +**Recommendation**: Display tour privacy icon alongside scene marker. + +--- + +## 10. Shared Link Token Validation + +### Current Implementation + +#### Token Generation (Tour Creation) +**File**: [backend/middlewares/TourController.js](backend/middlewares/TourController.js#L12-L35) + +```javascript +const newTour = new Tour({ + privacy: privacy || 'private', + shareToken: (privacy === 'shared') ? crypto.randomBytes(24).toString('hex') : undefined +}); + +if (newTour.privacy === 'shared') { + const expires = new Date(); + expires.setDate(expires.getDate() + 7); // Default 7 days + newTour.shareTokenExpires = expires; +} +``` + +**Status**: ✅ Correctly implemented with default 7-day expiration + +#### Token Validation (Multiple Endpoints) + +**Pattern Used**: +```javascript +const isTokenValid = tour.shareToken && + (!tour.shareTokenExpires || new Date() < tour.shareTokenExpires); + +if (tour.privacy === 'shared' && req.query.token === tour.shareToken && isTokenValid) + // Grant access +``` + +**Issues**: + +#### ISSUE 10.1 - Token Comparison Uses Weak Equality +**Severity**: MEDIUM + +All token comparisons use `===` (strict equality) which is good, but **no case normalization or encoding validation**. + +**Potential Issue**: +- Token with uppercase/lowercase differences won't match (probably fine) +- Token with URL encoding might not match (real issue) + +**Scenario**: +1. User gets token from URL: `?token=abc%2Bdef` (URL encoded +) +2. Backend receives: `abc+def` (after URL decoding) +3. But comparison happens against raw token stored: might have encoding differences + +**Recommendation**: Always decode and normalize tokens before comparison. + +#### ISSUE 10.2 - Token Not Revoked on Privacy Change +**Severity**: MEDIUM + +When a tour changes from shared → private, the code sets `tour.shareToken = undefined`. But: + +```javascript +tour.shareToken = undefined; // Mongoose behavior: removes field +``` + +**Potential Issue**: Existing tokens might still work if there's a race condition or if clients cache the token. + +**Better Approach**: Should explicitly null the token and add revocation timestamp. + +--- + +## 11. Cross-Tour Link Security + +### Current Implementation + +When a guest with a shared token navigates through hotspots: + +**File**: [backend/routes/sceneRoutes.js](backend/routes/sceneRoutes.js#L194-L205) + +```javascript +// Bridge access logic +if (!hasAccess && req.query.token) { + const potentialParents = await Hotspot.find({ target_scene_id: scene._id }); + const authorizedParentExists = await Scene.exists({ + _id: { $in: potentialParents }, + shareToken: req.query.token + }); + if (authorizedParentExists) hasAccess = true; +} +``` + +### Issues Found + +#### ISSUE 11.1 - Bridge Access Doesn't Validate Cross-Tour Navigation +**Severity**: HIGH + +This allows guests to navigate from a shared scene in Tour A to a scene in Tour B if they're hotspot-linked, **even if Tour B is private**. + +**Scenario**: +1. User A creates private Tour A with shared Scene 1 +2. User B creates private Tour B with Scene 2 +3. User A adds hotspot link from Scene 1 → Scene 2 (cross-tour link) +4. Guest with token for Scene 1 can now see Scene 2 **even though Tour B is private** + +**Root Cause**: Bridge logic doesn't check target scene's tour privacy + +**Recommendation**: +```javascript +// When validating bridge access, also check target tour privacy +if (authorizedParentExists) { + // Check if target scene's tour is accessible + const targetTour = await Tour.findById(scene.tourId); + if (targetTour.privacy === 'public' || targetTour.createdBy === req.user._id) { + hasAccess = true; + } + // Only grant access if target tour is public or owned by user +} +``` + +--- + +## 12. Member Access Email Validation + +### Current Implementation + +**File**: [backend/middlewares/TourController.js](backend/middlewares/TourController.js#L110-L115) + +```javascript +(tour.privacy === 'member' && req.user && req.user._id && ( + tour.sharedWith.some(u => u.toString() === req.user._id.toString()) || + (userEmail && tour.sharedEmails.includes(userEmail)) +)) +``` + +### Issues Found + +#### ISSUE 12.1 - Email Case Sensitivity +**Severity**: LOW + +Email comparison is **case-sensitive** but emails should be case-insensitive. + +**Scenario**: +1. Tour shared with: `User@Example.com` +2. User logs in with: `user@example.com` +3. Access **denied** (incorrect) + +**Fix Required**: +```javascript +tour.sharedEmails.map(e => e.toLowerCase()).includes(userEmail.toLowerCase()) +``` + +#### ISSUE 12.2 - Empty Email Bypass +**Severity**: MEDIUM + +If user exists but has no email, the check might bypass: + +```javascript +userEmail && tour.sharedEmails.includes(userEmail) +``` + +This is actually safe (requires non-empty email), but unclear. + +--- + +## Summary Table + +| # | Issue | File | Severity | Type | Impact | +|----|-------|------|----------|------|--------| +| 1.1 | Token validation missing email check | TourController.js | HIGH | Logic | Member access inconsistent | +| 2.1 | Shared tokens not returned in list | TourController.js | CRITICAL | Feature | Guests cannot discover shared tours | +| 2.2 | Private tour filtering unclear | TourController.js | MEDIUM | Logic | Edge case in permission logic | +| 3.1 | **Scene model missing privacy field** | Scene.js | CRITICAL | Schema | Core data structure broken | +| 3.2 | Bridge access too permissive | sceneRoutes.js | HIGH | Security | Cross-tour access bypass | +| 4.1 | Individual scene privacy not checked | sceneRoutes.js | HIGH | Logic | Private scenes visible in public tour | +| 4.2 | Token expiration not validated | sceneRoutes.js | MEDIUM | Logic | Expired tokens still work | +| 4.3 | Null privacy values ignored | sceneRoutes.js | MEDIUM | Data | Old scenes not found | +| 8.1 | Asset endpoint uses undefined privacy | assetRoutes.js | CRITICAL | Schema | Images serve to wrong users | +| 8.2 | Asset bridge access not implemented | assetRoutes.js | MEDIUM | Feature | Incomplete implementation | +| 9.1 | Frontend trust issue | main_map.js | MEDIUM | UX | No validation fallback | +| 9.2 | Tour privacy not displayed | main_map.js | LOW | UX | Information missing | +| 10.2 | Token revocation incomplete | TourController.js | MEDIUM | Logic | Old tokens might work | +| 11.1 | Cross-tour bridge access insecure | sceneRoutes.js | HIGH | Security | Private tours accessible | +| 12.1 | Email case sensitivity | TourController.js | LOW | Logic | Email-based access fails | + +--- + +## Recommendations by Priority + +### CRITICAL (Fix Immediately) + +1. **Add `privacy` field to Scene schema** + ```javascript + privacy: { + type: String, + enum: ['public', 'private', 'member', 'shared'], + default: 'private' + } + ``` + +2. **Migrate existing scenes** to have proper privacy values (inherit from tour if not set) + +3. **Add token support to GET /api/tours** + ```javascript + if (req.query.token) { + // Find tour with matching token and valid expiration + } + ``` + +4. **Fix scene privacy list endpoint** to check individual scene privacy, not just tour + +### HIGH PRIORITY (Fix This Week) + +5. **Validate token expiration in GET /api/scenes** + - Add `shareTokenExpires` check to token query + +6. **Secure bridge access logic** + - Check target tour privacy before allowing cross-tour navigation + - Apply same logic to asset streaming + +7. **Validate individual scene privacy** in public tours + - Don't assume all scenes in public tour are public + +### MEDIUM PRIORITY (Fix This Sprint) + +8. **Token revocation** + - Keep history of revoked tokens + - Add timestamp field + +9. **Token parameter normalization** + - Validate and normalize before comparison + - Handle URL encoding properly + +10. **Email case-insensitivity** + - Convert to lowercase before comparison + +11. **Frontend privacy indicators** + - Show lock icons for private scenes + - Show tour privacy on markers + +### LOW PRIORITY (Nice to Have) + +12. Add comprehensive logging for privacy access decisions +13. Add audit trail for privacy changes +14. Add privacy migration tool for legacy data + +--- + +## Testing Recommendations + +### Test Scenarios + +```javascript +// Public tour access +✓ Guest sees public tour scenes +✓ Guest can view public tour details + +// Private tour access +✓ Creator sees own private tour +✓ Guest cannot see private tour +✓ Non-creator user cannot see private tour + +// Shared token access +✓ Guest with valid token sees shared tour +✓ Guest with expired token denied access +✓ Guest with invalid token denied access +✓ Guest with shared token cannot access private tours via hotlinks + +// Member access +✓ User in sharedWith list can access member tour +✓ User with matching email can access member tour +✓ Case-insensitive email matching works +✓ Non-member user cannot access member tour + +// Cross-tour linking +✓ Hotspot link respects target tour privacy +✓ Bridge access validates expiration +✓ Bridge access doesn't bypass tour privacy + +// Asset streaming +✓ Image available to authorized users +✓ Image denied to unauthorized users +✓ Token validation works for assets +✓ Watermarking respects privacy +``` + +--- + +## Conclusion + +The project has a **solid foundation** for privacy management at the Tour level with proper propagation to scenes. However, **critical structural issues** (missing Scene.privacy field) and **logic gaps** (token handling, bridge access security, individual scene privacy validation) compromise the implementation. + +The good news: Most issues can be fixed without major refactoring. The bad news: The missing Scene.privacy field requires a data migration. + +**Recommended Action**: +1. Add Scene.privacy field to schema immediately +2. Migrate existing data +3. Apply the fix recommendations in priority order +4. Run comprehensive security tests diff --git a/backend/middlewares/TourController.js b/backend/middlewares/TourController.js index 60a53aa..58a2334 100644 --- a/backend/middlewares/TourController.js +++ b/backend/middlewares/TourController.js @@ -142,13 +142,29 @@ router.get('/:id', optionalAuth, async (req, res) => { const isAdmin = req.user && req.user.role === 'admin'; const isTokenValid = tour.shareToken && (!tour.shareTokenExpires || new Date() < tour.shareTokenExpires); const userEmail = req.user ? req.user.email : null; + const token = req.query.token; - let hasAccess = tour.privacy === 'public' || isOwner || isAdmin || - (tour.privacy === 'shared' && req.query.token === tour.shareToken && isTokenValid) || - (tour.privacy === 'member' && req.user && req.user._id && ( - tour.sharedWith.some(u => u.toString() === req.user._id.toString()) || - (userEmail && tour.sharedEmails.includes(userEmail)) - )); + // [Security] Check permissions based on tour privacy + let hasAccess = tour.privacy === 'public' || isOwner || isAdmin; + + // Shared link access - check token validity + if (!hasAccess && tour.privacy === 'shared' && token && isTokenValid) { + hasAccess = token === tour.shareToken; + } + + // Member access - check if user is in sharedWith or sharedEmails + if (!hasAccess && tour.privacy === 'member' && req.user && req.user._id && req.user.role !== 'guest') { + hasAccess = true; + } + + // Fallback for specific shared members + if (!hasAccess && tour.privacy === 'member' && req.user && req.user._id) { + hasAccess = tour.sharedWith.some(u => u.toString() === req.user._id.toString()) || + (userEmail && tour.sharedEmails.some(email => email.toLowerCase() === userEmail.toLowerCase())); + } + + // Private tours - only owner and admin + // (hasAccess already set above if owner/admin) if (!hasAccess) return res.status(403).json({ message: 'Bạn không có quyền truy cập Tour này.' }); @@ -159,16 +175,18 @@ router.get('/:id', optionalAuth, async (req, res) => { }); // @route GET /api/tours -// @desc Lấy danh sách Tour công khai hoặc của chính mình +// @desc Lấy danh sách Tour công khai, member, shared (với token), hoặc của chính mình // @access Public/Private router.get('/', optionalAuth, async (req, res) => { try { + const { token } = req.query; let query = { privacy: 'public' }; if (req.user && req.user.role !== 'guest') { query = { $or: [ { privacy: 'public' }, + { privacy: 'member' }, { createdBy: req.user._id }, { privacy: 'member', sharedWith: req.user._id }, { privacy: 'member', sharedEmails: req.user.email } @@ -176,6 +194,24 @@ router.get('/', optionalAuth, async (req, res) => { }; } + // [Task 4.1] Support shared tours via token for guests + if (token) { + const tourWithToken = await Tour.findOne({ + shareToken: token, + privacy: 'shared', + $or: [{ shareTokenExpires: null }, { shareTokenExpires: { $gt: new Date() } }] + }).select('_id'); + + if (tourWithToken) { + query = { + $or: [ + query, + { _id: tourWithToken._id } + ] + }; + } + } + const tours = await Tour.find(query) .populate('createdBy', 'username') .populate({ diff --git a/backend/models/Scene.js b/backend/models/Scene.js index d24a318..f77e606 100644 --- a/backend/models/Scene.js +++ b/backend/models/Scene.js @@ -39,6 +39,11 @@ const sceneSchema = new mongoose.Schema({ enum: ['processing', 'completed', 'failed'], default: 'processing' }, + privacy: { + type: String, + enum: ['public', 'private', 'member', 'shared'], + default: 'private' + }, shareToken: String, shareTokenExpires: Date, sharedWith: [{ diff --git a/backend/routes/assetRoutes.js b/backend/routes/assetRoutes.js index a0db97c..dd99a15 100644 --- a/backend/routes/assetRoutes.js +++ b/backend/routes/assetRoutes.js @@ -24,6 +24,8 @@ const uploadDir = process.env.UPLOAD_DIR */ router.get('/assets/view/:assetId', verifyReferer, optionalAuth, async (req, res) => { try { + console.log(`[AssetView] Đang yêu cầu hiển thị Asset: ${req.params.assetId}. Token: ${req.query.token ? 'Có' : 'Không'}`); + const asset = await Asset.findById(req.params.assetId); if (!asset) return res.status(404).json({ message: 'Asset not found' }); @@ -88,7 +90,10 @@ router.get('/assets/view/:assetId', verifyReferer, optionalAuth, async (req, res } } - if (!hasAccess) return res.status(403).json({ message: 'Bạn không được phép di chuyển đến cảnh này' }); + if (!hasAccess) { + console.warn(`[AssetView Denied] Không có quyền xem ảnh cho Asset ${asset._id}`); + return res.status(403).json({ message: 'Bạn không được phép di chuyển đến cảnh này' }); + } } // Kiểm tra file vật lý (Giai đoạn 2 - Async) diff --git a/backend/routes/hotspotRoutes.js b/backend/routes/hotspotRoutes.js index 9a4e08e..12b26de 100644 --- a/backend/routes/hotspotRoutes.js +++ b/backend/routes/hotspotRoutes.js @@ -2,15 +2,47 @@ const express = require('express'); const router = express.Router(); const Hotspot = require('../models/Hotspot'); const Scene = require('../models/Scene'); -const { protect } = require('../middlewares/authMiddleware'); +const { protect, optionalAuth } = require('../middlewares/authMiddleware'); const { calculateReverseYaw } = require('../utils/hotspotHelper'); +const Tour = require('../models/Tour'); /** * @route GET /api/hotspots/:scene_id * @desc Lấy toàn bộ danh sách hotspot của một cảnh */ -router.get('/:scene_id', async (req, res) => { +router.get('/:scene_id', optionalAuth, async (req, res) => { try { + // [SECURITY] Kiểm tra quyền xem hotspots dựa trên quyền truy cập cảnh cha + const scene = await Scene.findById(req.params.scene_id).populate('tourId'); + if (!scene) return res.status(404).json({ message: 'Scene not found' }); + + const tour = scene.tourId; + const isOwner = req.user && req.user._id && tour?.createdBy?.toString() === req.user._id.toString(); + const isAdmin = req.user && req.user.role === 'admin'; + const isSceneTokenValid = scene.shareToken && (!scene.shareTokenExpires || new Date() < scene.shareTokenExpires); + const isTourTokenValid = tour?.shareToken && (!tour.shareTokenExpires || new Date() < tour.shareTokenExpires); + + let hasAccess = (tour?.privacy === 'public') || (scene.privacy === 'public') || isOwner || isAdmin || + (scene.privacy === 'shared' && req.query.token === scene.shareToken && isSceneTokenValid) || + (tour?.privacy === 'shared' && req.query.token === tour.shareToken && isTourTokenValid) || + (tour?.privacy === 'member' && req.user && req.user._id && req.user.role !== 'guest'); + + // Bridge Access cho Hotspots + if (!hasAccess && req.query.token) { + const potentialParents = await Hotspot.find({ target_scene_id: scene._id }).distinct('parent_scene_id'); + const authorizedParentExists = await Scene.exists({ + _id: { $in: potentialParents }, + shareToken: req.query.token, + $or: [{ shareTokenExpires: null }, { shareTokenExpires: { $gt: new Date() } }] + }); + if (authorizedParentExists) hasAccess = true; + } + + if (!hasAccess) { + console.warn(`[Backend-Hotspots-Denied] Scene: ${req.params.scene_id}`); + return res.status(403).json({ message: 'Bạn không có quyền xem các điểm điều hướng của cảnh này.' }); + } + const hotspots = await Hotspot.find({ parent_scene_id: req.params.scene_id }) .populate({ path: 'target_scene_id', diff --git a/backend/routes/sceneRoutes.js b/backend/routes/sceneRoutes.js index fd93f88..5612184 100644 --- a/backend/routes/sceneRoutes.js +++ b/backend/routes/sceneRoutes.js @@ -117,7 +117,16 @@ router.get('/', optionalAuth, async (req, res) => { // Quyền cơ bản: Công khai hoặc là chủ sở hữu/thành viên được chia sẻ let baseQuery = req.user && req.user.role !== 'guest' - ? { $or: [{ privacy: 'public' }, { tourId: { $in: publicTourIds } }, { createdBy: req.user._id }, { sharedWith: req.user._id }, { sharedEmails: req.user.email }] } + ? { + $or: [ + { privacy: 'public' }, + { privacy: 'member' }, + { tourId: { $in: publicTourIds } }, + { createdBy: req.user._id }, + { sharedWith: req.user._id }, + { sharedEmails: req.user.email } + ] + } : { $or: [{ privacy: 'public' }, { tourId: { $in: publicTourIds } }] }; let finalQuery = baseQuery; @@ -148,6 +157,8 @@ router.get('/', optionalAuth, async (req, res) => { // @route GET /api/scenes/:id router.get('/:id', optionalAuth, async (req, res) => { try { + console.log(`[Backend-Scene] Yêu cầu chi tiết: ${req.params.id}. User: ${req.user?._id || 'Guest'}, QueryToken: ${req.query.token || 'N/A'}`); + const scene = await Scene.findById(req.params.id) .populate('createdBy', 'username') .populate('assetId') @@ -165,18 +176,23 @@ router.get('/:id', optionalAuth, async (req, res) => { const isTourTokenValid = tour.shareToken && (!tour.shareTokenExpires || new Date() < tour.shareTokenExpires); const userEmail = req.user ? req.user.email : null; - let hasAccess = tour.privacy === 'public' || isOwner || isAdmin || + // [FIX] Cho phép truy cập nếu bản thân Scene CÔNG KHAI hoặc Tour CÔNG KHAI + let hasAccess = tour.privacy === 'public' || scene.privacy === 'public' || isOwner || isAdmin || (scene.privacy === 'shared' && req.query.token === scene.shareToken && isSceneTokenValid) || // Access via scene's token (tour.privacy === 'shared' && req.query.token === tour.shareToken && isTourTokenValid) || // Access via tour's token - (tour.privacy === 'member' && req.user && req.user._id && ( // Access for members + (tour.privacy === 'member' && req.user && req.user._id && req.user.role !== 'guest') || // Access for any logged-in member + (tour.privacy === 'member' && req.user && req.user._id && ( // Specific shared members (legacy support) tour.sharedWith.some(u => u.toString() === req.user._id.toString()) || (userEmail && tour.sharedEmails.includes(userEmail)) )); + if (req.query.token) { + console.log(`[Backend-Auth] Token: ${req.query.token}. Match Scene: ${req.query.token === scene.shareToken}, Match Tour: ${req.query.token === tour.shareToken}, Access: ${hasAccess}`); + } + // [BRIDGE ACCESS LOGIC] // Nếu chưa có quyền, kiểm tra xem người dùng có đến từ một cảnh hợp lệ thuộc Tour khác không if (!hasAccess && req.query.token) { - // Tìm tất cả các cảnh (parent) có hotspot trỏ đến cảnh hiện tại const potentialParents = await Hotspot.find({ target_scene_id: scene._id }).distinct('parent_scene_id'); if (potentialParents.length > 0) { // Kiểm tra xem có cảnh cha nào sở hữu shareToken này và còn hạn không @@ -185,11 +201,17 @@ router.get('/:id', optionalAuth, async (req, res) => { shareToken: req.query.token, $or: [{ shareTokenExpires: null }, { shareTokenExpires: { $gt: new Date() } }] }); - if (authorizedParentExists) hasAccess = true; + if (authorizedParentExists) { + hasAccess = true; + console.log(`[Backend-Bridge] Quyền được chấp thuận qua Scene cha.`); + } } } - if (!hasAccess) return res.status(403).json({ message: 'Bạn không được phép di chuyển đến cảnh này' }); + if (!hasAccess) { + console.warn(`[Backend-Denied] Scene: ${scene._id}, TourPrivacy: ${tour.privacy}, ScenePrivacy: ${scene.privacy}`); + return res.status(403).json({ message: 'Bạn không có quyền truy cập cảnh này.' }); + } // Increment view count if not owner/admin and not a bot if (!isOwner && !isAdmin && !req.headers['user-agent']?.match(/bot|crawl|spider/i)) { diff --git a/frontend/js/main_map.js b/frontend/js/main_map.js index b73151f..9dbf982 100644 --- a/frontend/js/main_map.js +++ b/frontend/js/main_map.js @@ -993,14 +993,6 @@ async function loadScenes(urlToken = null) { return; } - // 2. Logic lọc Ảnh mẹ: Sửa lỗi typo coordKey (dùng latNum 2 lần) - const coordKey = `${latNum.toFixed(6)},${lngNum.toFixed(6)}`; - if (seenCoordinates.has(coordKey)) { - console.log(`[Frontend] Bỏ qua Scene "${scene.name || scene.title}" (ID: ${scene._id}) do trùng tọa độ (hotspot con).`); - return; - } - seenCoordinates.add(coordKey); - // 3. Truy cập Asset an toàn const assetId = scene.assetId?._id || scene.assetId; if (!assetId) return; // Bỏ qua nếu không có ảnh liên kết @@ -1306,11 +1298,10 @@ async function openScene(sceneId, privacy, shareToken, force = false, initialPit headers['Authorization'] = `Bearer ${token}`; } - console.log(`[Viewer] Đang mở scene: ${sceneId}`); - let url = `${API_BASE_URL}/scenes/${sceneId}`; - if (privacy === 'shared' && shareToken) { - url += `?token=${shareToken}`; - } + console.log(`[Frontend-Viewer] Đang nạp Scene: ${sceneId}. Token: ${shareToken || 'None'}`); + const authParam = shareToken ? `?token=${shareToken}` : ''; + let url = `${API_BASE_URL}/scenes/${sceneId}${authParam}`; + let hotspotsUrl = `${API_BASE_URL}/hotspots/${sceneId}${authParam}`; // Lưu trạng thái Scene hiện tại để khôi phục sau khi reload trang localStorage.setItem('activeSceneId', sceneId); @@ -1320,23 +1311,20 @@ async function openScene(sceneId, privacy, shareToken, force = false, initialPit // Nạp đồng thời Scene và danh sách Hotspots từ Collection riêng const [sceneRes, hotspotsRes] = await Promise.all([ fetch(url, { method: 'GET', headers }), - fetch(`${API_BASE_URL}/hotspots/${sceneId}`, { method: 'GET', headers }) + fetch(hotspotsUrl, { method: 'GET', headers }) ]); + if (!sceneRes.ok) { + const errorData = await sceneRes.json(); + console.error(`[Viewer API Error] Scene status: ${sceneRes.status}. Message: ${errorData.message}`); + throw new Error(errorData.message || 'Không thể tải thông tin cảnh'); + } + console.log(`[Viewer API Success] Đã nạp xong Scene và Hotspots cho ${sceneId}`); + const scene = await sceneRes.json(); const hotspots = await hotspotsRes.json(); // Ngăn chặn mở Scene nếu ảnh chưa xử lý xong hoặc lỗi - if (scene.status === 'processing') { - showNotification("Cảnh này đang được nén chất lượng 8K. Vui lòng quay lại sau vài giây.", 'warning'); - return; - } - if (scene.status === 'failed') { - showNotification("Lỗi xử lý ảnh 8K. Vui lòng upload lại ảnh cho cảnh này.", 'error'); - return; - } - - // [TOUR ID] Luôn cập nhật activeTourId theo Scene hiện tại để đảm bảo các cảnh con/hotspot mới // được gán đúng vào Tour gốc của cảnh đang xem, tránh sử dụng ID cũ/lỗi từ phiên trước. const openedSceneTourId = scene.tourId?._id || scene.tourId || scene._id; localStorage.setItem('activeTourId', openedSceneTourId); @@ -1351,10 +1339,6 @@ async function openScene(sceneId, privacy, shareToken, force = false, initialPit const isOwner = currentUserId && sceneOwnerId && currentUserId.toString() === sceneOwnerId.toString(); const isAdmin = userRole === 'admin' || userRole === 'Chủ sở hữu'; - if (scene.privacy === 'private' && !isOwner && !isAdmin) { - throw new Error("Cảnh này đã được chủ sở hữu chuyển sang chế độ riêng tư."); - } - console.log("DEBUG: Hotspots raw data from API:", hotspots); // Tự động focus bản đồ vào vị trí của Scene @@ -1406,10 +1390,11 @@ async function openScene(sceneId, privacy, shareToken, force = false, initialPit // Kiểm tra nếu đang truy cập qua link trực tiếp (URL có sceneId) mà gặp lỗi (do xóa token hoặc token không hợp lệ) const urlParams = new URLSearchParams(window.location.search); if (urlParams.has('sceneId') || window.location.pathname.includes('/api/share/')) { - showNotification("Bạn không có quyền truy cập hoặc liên kết chia sẻ đã hết hạn. Quay về bản đồ công cộng.", 'error'); - // Xóa toàn bộ tham số URL và tải lại trang để làm mới trạng thái (về trang chủ dành cho khách) + // Sử dụng Error Modal để thông báo không bị biến mất đột ngột + showErrorModal("Bạn không có quyền truy cập hoặc liên kết chia sẻ đã hết hạn. Quay về bản đồ công cộng.", "Lỗi truy cập", "🚫"); + + // Chỉ xóa tham số URL để làm sạch thanh địa chỉ, không cần reload trang gây văng marker window.history.replaceState({}, document.title, "/"); - location.reload(); return; }