From 4dc6e0526da4a4d3a05b98c15fce47247762b337 Mon Sep 17 00:00:00 2001 From: Developer Date: Sun, 20 Sep 2026 02:35:37 -0400 Subject: [PATCH] fix(web): address clubs review findings - Gate ranks-roles management route on canManageSettings with safe unauthorized state, matching sibling founder-management routes - Enforce HTTPS image URL invariant for club logo/banner in the data layer (validateClubImageUrl) and in Firestore rules, preserving null/empty clearing --- .../emulator-tests/firestore.rules.test.js | 114 +++++++++++++++++ firestore.rules | 14 ++ .../[clubId]/manage/ranks-roles/page.tsx | 27 ++++ website/lib/club-data.ts | 32 ++++- website/tests/founder-portal.test.cjs | 121 +++++++++++++++++- 5 files changed, 305 insertions(+), 3 deletions(-) diff --git a/firebase/emulator-tests/firestore.rules.test.js b/firebase/emulator-tests/firestore.rules.test.js index 32ffd4a..2ba59c7 100644 --- a/firebase/emulator-tests/firestore.rules.test.js +++ b/firebase/emulator-tests/firestore.rules.test.js @@ -3013,6 +3013,120 @@ describe('Social Architecture Security Rules', () => { })); }); + test('FP-14.3: Club logo/banner HTTPS URL invariant enforced at the storage layer', async () => { + const alice = testEnv.authenticatedContext('alice').firestore(); + const bob = testEnv.authenticatedContext('bob').firestore(); + const charlie = testEnv.authenticatedContext('charlie').firestore(); + + await testEnv.withSecurityRulesDisabled(async (context) => { + const db = context.firestore(); + await setDoc(doc(db, 'clubs/club_image_url_sec'), { + id: 'club_image_url_sec', + name: 'Image URL Security MC', + slug: 'image-url-security-mc', + visibility: 'public', + admissionPolicy: 'request', + ownerUid: 'alice', + memberCount: 3, + archivedAt: null, + createdAt: new Date(), + updatedAt: new Date(), + }); + await setDoc(doc(db, 'clubs/club_image_url_sec/members/alice'), { + uid: 'alice', + clubId: 'club_image_url_sec', + role: 'owner', + status: 'active', + }); + await setDoc(doc(db, 'clubs/club_image_url_sec/members/bob'), { + uid: 'bob', + clubId: 'club_image_url_sec', + role: 'admin', + status: 'active', + }); + await setDoc(doc(db, 'clubs/club_image_url_sec/members/charlie'), { + uid: 'charlie', + clubId: 'club_image_url_sec', + role: 'member', + status: 'active', + displayRank: 'President', + }); + }); + + const clubDocRef = (db) => doc(db, 'clubs/club_image_url_sec'); + + // 1. Owner sets valid https logo/banner -> ALLOW + await assertSucceeds(updateDoc(clubDocRef(alice), { + logoUrl: 'https://example.com/logo.png', + bannerUrl: 'https://cdn.example.com/banner.jpg', + updatedAt: new Date(), + })); + + // 2. Owner clears logo with null and banner with empty string -> ALLOW + await assertSucceeds(updateDoc(clubDocRef(alice), { + logoUrl: null, + updatedAt: new Date(), + })); + await assertSucceeds(updateDoc(clubDocRef(alice), { + bannerUrl: '', + updatedAt: new Date(), + })); + + // 3. Admin sets valid https URLs -> ALLOW + await assertSucceeds(updateDoc(clubDocRef(bob), { + logoUrl: 'https://example.com/admin-logo.png', + updatedAt: new Date(), + })); + + // 4. Owner http:// URL -> DENY (insecure scheme) + await assertFails(updateDoc(clubDocRef(alice), { + logoUrl: 'http://evil.example.com/logo.png', + updatedAt: new Date(), + })); + + // 5. Owner data: URL -> DENY + await assertFails(updateDoc(clubDocRef(alice), { + logoUrl: 'data:image/png;base64,iVBORw0KGgo=', + updatedAt: new Date(), + })); + + // 6. Owner blob: URL -> DENY + await assertFails(updateDoc(clubDocRef(alice), { + bannerUrl: 'blob:https://example.com/fake-uuid', + updatedAt: new Date(), + })); + + // 7. Owner javascript: URL -> DENY + await assertFails(updateDoc(clubDocRef(alice), { + bannerUrl: 'javascript:alert(1)', + updatedAt: new Date(), + })); + + // 8. Owner malformed value -> DENY + await assertFails(updateDoc(clubDocRef(alice), { + logoUrl: 'not-a-url', + updatedAt: new Date(), + })); + + // 9. Owner non-string URL value -> DENY + await assertFails(updateDoc(clubDocRef(alice), { + logoUrl: 42, + updatedAt: new Date(), + })); + + // 10. Admin http:// URL -> DENY (same invariant for admins) + await assertFails(updateDoc(clubDocRef(bob), { + bannerUrl: 'http://evil.example.com/banner.jpg', + updatedAt: new Date(), + })); + + // 11. Regular member writes valid https URL -> DENY (no management authority at all) + await assertFails(updateDoc(clubDocRef(charlie), { + logoUrl: 'https://example.com/member-logo.png', + updatedAt: new Date(), + })); + }); + test('FP-15: Club Applications membership request approval and decline security matrix', async () => { const alice = testEnv.authenticatedContext('alice').firestore(); const bob = testEnv.authenticatedContext('bob').firestore(); diff --git a/firestore.rules b/firestore.rules index 673449f..17bd313 100644 --- a/firestore.rules +++ b/firestore.rules @@ -701,6 +701,16 @@ service cloud.firestore { return exists(/databases/$(database)/documents/clubs/$(clubId)/bans/$(uid)); } + // Club logo/banner URLs: null or empty string clears; otherwise must be + // an https:// URL (case-insensitive scheme, matching URL() semantics in the app layer). + function isValidClubImageUrl(url) { + return url == null + || url == '' + || (url is string + && url.size() <= 2048 + && url.matches('(?i)^https://\\S+$')); + } + function isValidMemberCountIncrement() { let targetUid = ('lastMembershipChangeUid' in request.resource.data) ? request.resource.data.lastMembershipChangeUid @@ -779,6 +789,8 @@ service cloud.firestore { && (!('name' in request.resource.data) || (request.resource.data.name is string && request.resource.data.name.size() >= 3 && request.resource.data.name.size() <= 50)) && (!('visibility' in request.resource.data) || request.resource.data.visibility in ['public', 'unlisted', 'private']) && (!('admissionPolicy' in request.resource.data) || request.resource.data.admissionPolicy in ['open', 'request', 'inviteOnly']) + && (!('logoUrl' in request.resource.data) || isValidClubImageUrl(request.resource.data.logoUrl)) + && (!('bannerUrl' in request.resource.data) || isValidClubImageUrl(request.resource.data.bannerUrl)) ) || // Case 3: Admin editing metadata and access settings (cannot change ownerUid, slug, createdAt, archivedAt, memberCount) ( @@ -790,6 +802,8 @@ service cloud.firestore { && (!('name' in request.resource.data) || (request.resource.data.name is string && request.resource.data.name.size() >= 3 && request.resource.data.name.size() <= 50)) && (!('visibility' in request.resource.data) || request.resource.data.visibility in ['public', 'unlisted', 'private']) && (!('admissionPolicy' in request.resource.data) || request.resource.data.admissionPolicy in ['open', 'request', 'inviteOnly']) + && (!('logoUrl' in request.resource.data) || isValidClubImageUrl(request.resource.data.logoUrl)) + && (!('bannerUrl' in request.resource.data) || isValidClubImageUrl(request.resource.data.bannerUrl)) ) || // Case 4: Member count increment/decrement by membership transitions ONLY ( diff --git a/website/app/clubs/[clubId]/manage/ranks-roles/page.tsx b/website/app/clubs/[clubId]/manage/ranks-roles/page.tsx index 9a50174..79da603 100644 --- a/website/app/clubs/[clubId]/manage/ranks-roles/page.tsx +++ b/website/app/clubs/[clubId]/manage/ranks-roles/page.tsx @@ -26,17 +26,20 @@ export default function ClubFounderRanksRolesPage({ params }: PageProps) { const [loading, setLoading] = useState(true); const [error, setError] = useState(null); const [unauthenticated, setUnauthenticated] = useState(false); + const [unauthorized, setUnauthorized] = useState(false); async function loadData() { setLoading(true); setError(null); setUnauthenticated(false); + setUnauthorized(false); try { await firebaseAuth.authStateReady(); const currentUser = firebaseAuth.currentUser; const isDemoPreview = searchParams?.get('preview') === 'demo'; + // Unauthenticated Guard: Direct access without demo preview requires authentication if (!isDemoPreview && !currentUser) { setUnauthenticated(true); setLoading(false); @@ -47,6 +50,13 @@ export default function ClubFounderRanksRolesPage({ params }: PageProps) { allowDemoPreview: isDemoPreview, }); + // Authorization Guard: Owner and Admin can access Ranks & Roles; Members cannot + if (!isDemoPreview && !ctx.permissions.canManageSettings) { + setUnauthorized(true); + setLoading(false); + return; + } + setContext(ctx); } catch (err) { setError(sanitizePortalError(err, "Ranks and roles couldn't be loaded.")); @@ -69,6 +79,7 @@ export default function ClubFounderRanksRolesPage({ params }: PageProps) { ); } + // Unauthenticated safe error state if (unauthenticated) { return (
@@ -84,6 +95,22 @@ export default function ClubFounderRanksRolesPage({ params }: PageProps) { ); } + // Unauthorized (Member or non-admin) safe error state + if (unauthorized) { + return ( +
+
+ router.push('/clubs')} + /> +
+
+ ); + } + if (error || !context) { return (
diff --git a/website/lib/club-data.ts b/website/lib/club-data.ts index fc1f064..6d01997 100644 --- a/website/lib/club-data.ts +++ b/website/lib/club-data.ts @@ -613,6 +613,30 @@ export interface UpdateClubProfileInput { bannerUrl?: string; } +/** + * Shared image URL validation for club logo/banner persistence. + * Accepts https:// URLs or blank values (clearing). Returns the trimmed + * URL, or null when the field should be cleared. Never normalizes an + * invalid URL into a valid one; throws on http://, data:, blob:, + * javascript:, or malformed values. + */ +export function validateClubImageUrl(value: string, fieldLabel: string): string | null { + const trimmed = (value ?? '').trim(); + if (!trimmed) { + return null; + } + let parsed: URL; + try { + parsed = new URL(trimmed); + } catch { + throw new Error(`${fieldLabel} must be a valid HTTPS URL.`); + } + if (parsed.protocol !== 'https:') { + throw new Error(`${fieldLabel} must use a secure https:// URL.`); + } + return trimmed; +} + /** * Whitelist update of club public profile metadata. * Strictly verifies caller is owner or admin. @@ -647,6 +671,10 @@ export async function updateClubProfile( throw new Error('Motorcycle focus must not exceed 100 characters.'); } + const logoUrl = input.logoUrl !== undefined ? validateClubImageUrl(input.logoUrl, 'Club logo URL') : undefined; + const bannerUrl = + input.bannerUrl !== undefined ? validateClubImageUrl(input.bannerUrl, 'Club banner URL') : undefined; + const clubRef = doc(firestore, 'clubs', clubId); const clubSnap = await getDoc(clubRef); if (!clubSnap.exists()) { @@ -683,8 +711,8 @@ export async function updateClubProfile( description, homeRegion, motorcycleFocus, - ...(input.logoUrl !== undefined ? { logoUrl: input.logoUrl.trim() || null } : {}), - ...(input.bannerUrl !== undefined ? { bannerUrl: input.bannerUrl.trim() || null } : {}), + ...(logoUrl !== undefined ? { logoUrl } : {}), + ...(bannerUrl !== undefined ? { bannerUrl } : {}), updatedAt: serverTimestamp(), }); } diff --git a/website/tests/founder-portal.test.cjs b/website/tests/founder-portal.test.cjs index 51b2c40..ca5ddaf 100644 --- a/website/tests/founder-portal.test.cjs +++ b/website/tests/founder-portal.test.cjs @@ -69,6 +69,7 @@ const { fetchClubApplications, approveClubApplication, declineClubApplication, + validateClubImageUrl, } = clubData; const { FIXTURE_FOUNDER_CLUB, FIXTURE_METRICS } = clubFixtures; @@ -828,7 +829,125 @@ async function runAllTests() { console.log(' PASS: FP-15 Tests A through V verified (Route exists, Owner/Admin access, Member denial, Unauth denial, Bounded query limit 25, Status requested only, Approve requested->active, Decline requested->declined, Default role member, Default rank Member, memberCount +1 atomic, memberCount delta 0 decline, Ban rejection, Stale request protection, Duplicate click guard, Zero rank privilege, Zero private telemetry/contact leak, Production fixture isolation, Sidebar routing & active state, TopBar breadcrumb, Non-destructive history preservation).'); - console.log('--- ALL FOUNDER PORTAL TESTS PASSED (23/23) ---'); + // ========================================================================= + // TEST 24: Codex Review Findings — Ranks-Roles Access Guard + HTTPS Image URL Invariant + // ========================================================================= + console.log('Testing: Codex findings — Ranks-Roles access guard & HTTPS image URL invariant...'); + + // --- 24A: Ranks & Roles route enforces canManageSettings guard --- + const ranksRolesPageSrc = fs.readFileSync(path.resolve(__dirname, '../app/clubs/[clubId]/manage/ranks-roles/page.tsx'), 'utf8'); + + // Owner/Admin allowed by the established permission model + assert.equal(getPortalPermissions('owner', true).canManageSettings, true, 'Owner must have canManageSettings'); + assert.equal(getPortalPermissions('admin', true).canManageSettings, true, 'Admin must have canManageSettings'); + + // Ordinary member denied + assert.equal(getPortalPermissions('member', true).canManageSettings, false, 'Member must not have canManageSettings'); + + // Unauthenticated denied + assert.equal(getPortalPermissions('owner', false).isAuthenticated, false, 'Unauthenticated user must have isAuthenticated = false'); + assert.equal(getPortalPermissions('owner', false).canManageSettings, false, 'Unauthenticated user must not have canManageSettings'); + + // Route source: unauthenticated guard, authorization guard, safe unauthorized state, demo-preview preserved + assert(ranksRolesPageSrc.includes("if (!isDemoPreview && !currentUser)"), 'Ranks-Roles page must guard unauthenticated access'); + assert(ranksRolesPageSrc.includes('title="Authentication Required"'), 'Ranks-Roles page must render Authentication Required safe state'); + assert(ranksRolesPageSrc.includes('!ctx.permissions.canManageSettings'), 'Ranks-Roles page must enforce canManageSettings gate'); + assert(ranksRolesPageSrc.includes('setUnauthorized(true)'), 'Ranks-Roles page must set unauthorized state'); + assert(ranksRolesPageSrc.includes('title="Management Access Restricted"'), 'Ranks-Roles page must render unauthorized safe state'); + assert(ranksRolesPageSrc.includes('allowDemoPreview: isDemoPreview'), 'Ranks-Roles page must preserve demo-preview behavior'); + + // --- 24B: validateClubImageUrl shared data-layer validator --- + // Accepted: valid https URLs (returns trimmed value) + assert.equal(validateClubImageUrl('https://example.com/logo.png', 'Club logo URL'), 'https://example.com/logo.png'); + assert.equal(validateClubImageUrl(' https://cdn.example.com/b banner.jpg ', 'Club banner URL'), 'https://cdn.example.com/b banner.jpg'); + + // Accepted: blank/empty values clear the field (returns null) + assert.equal(validateClubImageUrl('', 'Club logo URL'), null); + assert.equal(validateClubImageUrl(' ', 'Club banner URL'), null); + + // Rejected: http://, data:, blob:, javascript:, ftp:, malformed + const rejectedUrls = [ + 'http://example.com/logo.png', + 'data:image/png;base64,iVBORw0KGgo=', + 'blob:https://example.com/uuid', + 'javascript:alert(1)', + 'ftp://example.com/logo.png', + 'not-a-url', + 'example.com/logo.png', + 'https://', + ]; + for (const bad of rejectedUrls) { + let threw = false; + try { + validateClubImageUrl(bad, 'Club logo URL'); + } catch (err) { + threw = true; + assert( + err.message === 'Club logo URL must be a valid HTTPS URL.' || err.message === 'Club logo URL must use a secure https:// URL.', + `Unexpected validation error message for ${bad}: ${err.message}` + ); + } + assert.equal(threw, true, `validateClubImageUrl must reject: ${bad}`); + } + + // Never silently normalizes an invalid URL into a valid one + assert.equal(validateClubImageUrl('https://example.com/x.png', 'Club logo URL'), 'https://example.com/x.png'); + + // --- 24C: updateClubProfile invokes validator before Firestore reads --- + const updateProfileBody = clubDataSrc.substring(clubDataSrc.indexOf('export async function updateClubProfile'), clubDataSrc.indexOf('export interface UpdateClubAccessSettingsInput')); + assert(updateProfileBody.includes('validateClubImageUrl(input.logoUrl'), 'updateClubProfile must validate logoUrl via validateClubImageUrl'); + assert(updateProfileBody.includes('validateClubImageUrl(input.bannerUrl'), 'updateClubProfile must validate bannerUrl via validateClubImageUrl'); + assert(updateProfileBody.indexOf('validateClubImageUrl(input.logoUrl') < updateProfileBody.indexOf('getDoc(clubRef)'), 'URL validation must run before Firestore reads'); + assert(!updateProfileBody.includes('input.logoUrl.trim()'), 'updateClubProfile must not write unvalidated logoUrl'); + assert(!updateProfileBody.includes('input.bannerUrl.trim()'), 'updateClubProfile must not write unvalidated bannerUrl'); + + // Behavioral: invalid URL rejected before any Firestore read (mocked getDoc never resolves a club) + let urlThrew = false; + try { + await updateClubProfile('any-club', 'caller-uid', { + name: 'Valid Club Name', + description: '', + homeRegion: '', + motorcycleFocus: '', + logoUrl: 'http://evil.example.com/x.png', + }); + } catch (err) { + urlThrew = true; + assert.equal(err.message, 'Club logo URL must use a secure https:// URL.'); + } + assert.equal(urlThrew, true, 'updateClubProfile must reject http:// logoUrl before touching Firestore'); + + // Behavioral: valid input with no URL fields reaches the not-found read (proves validation did not throw) + let notFoundThrew = false; + try { + await updateClubProfile('any-club', 'caller-uid', { + name: 'Valid Club Name', + description: '', + homeRegion: '', + motorcycleFocus: '', + logoUrl: 'https://example.com/ok.png', + bannerUrl: '', + }); + } catch (err) { + notFoundThrew = true; + assert.equal(err.message, 'Club not found.', `Expected Firestore read after passing validation, got: ${err.message}`); + } + assert.equal(notFoundThrew, true, 'updateClubProfile with valid URLs must proceed to Firestore read'); + + // --- 24D: Firestore rules enforce the same invariant at the storage layer --- + const rulesSrc = fs.readFileSync(path.resolve(__dirname, '../../firestore.rules'), 'utf8'); + assert(rulesSrc.includes('function isValidClubImageUrl(url)'), 'firestore.rules must define isValidClubImageUrl'); + assert(rulesSrc.includes("url.matches('(?i)^https://\\\\S+$')"), 'firestore.rules must require https:// scheme'); + const rulesCase2 = rulesSrc.substring(rulesSrc.indexOf('// Case 2: Owner general metadata update'), rulesSrc.indexOf('// Case 3: Admin editing metadata')); + const rulesCase3 = rulesSrc.substring(rulesSrc.indexOf('// Case 3: Admin editing metadata'), rulesSrc.indexOf('// Case 4: Member count')); + for (const [label, block] of [['Case 2 (owner)', rulesCase2], ['Case 3 (admin)', rulesCase3]]) { + assert(block.includes("isValidClubImageUrl(request.resource.data.logoUrl)"), `firestore.rules ${label} must validate logoUrl`); + assert(block.includes("isValidClubImageUrl(request.resource.data.bannerUrl)"), `firestore.rules ${label} must validate bannerUrl`); + } + + console.log(' PASS: Ranks-Roles canManageSettings guard verified (owner/admin allowed, member/unauth denied, demo-preview preserved); HTTPS image URL invariant enforced in app layer and Firestore rules (https accepted, blank clears, http/data/blob/javascript/malformed rejected).'); + + console.log('--- ALL FOUNDER PORTAL TESTS PASSED (24/24) ---'); } runAllTests().catch((err) => {