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
This commit is contained in:
Developer
parent
008a5fc5bd
commit
4dc6e0526d
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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
|
||||
(
|
||||
|
|
|
|||
|
|
@ -26,17 +26,20 @@ export default function ClubFounderRanksRolesPage({ params }: PageProps) {
|
|||
const [loading, setLoading] = useState(true);
|
||||
const [error, setError] = useState<string | null>(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 (
|
||||
<div className="min-h-screen bg-brand-bg telemetry-grid-bg flex items-center justify-center p-6">
|
||||
|
|
@ -84,6 +95,22 @@ export default function ClubFounderRanksRolesPage({ params }: PageProps) {
|
|||
);
|
||||
}
|
||||
|
||||
// Unauthorized (Member or non-admin) safe error state
|
||||
if (unauthorized) {
|
||||
return (
|
||||
<div className="min-h-screen bg-brand-bg telemetry-grid-bg flex items-center justify-center p-6">
|
||||
<div className="max-w-5xl mx-auto w-full flex items-center justify-center">
|
||||
<SafeErrorState
|
||||
title="Management Access Restricted"
|
||||
message="You must be an Owner or Admin of this Club to access Ranks & Roles."
|
||||
actionLabel="Return to Clubs"
|
||||
onAction={() => router.push('/clubs')}
|
||||
/>
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
if (error || !context) {
|
||||
return (
|
||||
<div className="min-h-screen bg-brand-bg telemetry-grid-bg flex items-center justify-center p-6">
|
||||
|
|
|
|||
|
|
@ -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(),
|
||||
});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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) => {
|
||||
|
|
|
|||
Loading…
Reference in New Issue