feat: add auth flow and role-based views (admin vs stakeholder) #5
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "4-auth-flow-role-based-views"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Adds a signin page with demo account links that route users into role-specific views. Admin sees all buckets with full CRUD controls. Stakeholders are scoped to their project prefix within the assets bucket -- read and upload only, no delete. Role state persists via sessionStorage and URL params.
Changes
signin.html(new) -- Login form (non-functional) with 3 demo account links: Admin, Stakeholder (Westside), Stakeholder (MCD Tracker). Includes@route,@auth,@api,@interactivity,@gaps,@notesHTML comments for SvelteKit promotion.app.js-- Added role state management (sessionStorage),signOut(),renderBottomNav()(role-aware),renderRoleHeader()(project name banner),isAllowedPath()access control. Updated all page renderers to enforce stakeholder prefix scoping, hide delete buttons for stakeholders, and redirect unauthenticated users to signin. Added mock data forassets/westside/andassets/mcd-tracker/prefixes.style.css-- Added signin page styles (centered card, form inputs, demo links, divider), role banner styles (admin/stakeholder badges, project name, sign-out button). No horizontal scroll at 390px.index.html-- Updated bottom nav to include Settings link for admin. Stakeholders are redirected to their scoped browse view by JS.browse.html,upload.html,preview.html,detail.html-- Updated bottom nav to be dynamically rendered by app.js based on role.Test Plan
signin.htmlat 390px -- verify centered card layout with form and 3 demo linksindex.htmlwith all 3 buckets visible, nav shows Buckets/Upload/Settingsbrowse.htmlscoped toassets/westside/, nav shows My Files/Upload, header shows "Westside Basketball"browse.htmlscoped toassets/mcd-tracker/, header shows "MCD Tracker"Review Checklist
node --checkpython3 -m http.server 8080Related
plan-minio-mobile(traceability)QA Review -- PR #5
Scope Check
PR touches 8 files (1 new, 7 modified). All changes are scoped to issue #4 (auth flow + role-based views). No unrelated changes detected.
Findings
1. [nit] index.html has redundant static nav links
index.htmlstill has hardcoded nav links inside<nav class="bottom-nav">alongside the<!-- Rendered by app.js based on role -->comment. The JSrenderBottomNav()replacesinnerHTML, so the static content will flash briefly before JS executes (FOUC). Other pages (browse, upload, preview, detail) correctly have only the comment placeholder. Recommend removing the static links fromindex.htmlto be consistent.2. [nit] XSS in
renderRoleHeader()via project paramThe
projectvalue comes from URL params and is inserted viainnerHTMLwithout sanitization. A crafted URL like?project=<img src=x onerror=alert(1)>would execute script. Acceptable for a prototype, but worth a// TODO: sanitizecomment for SvelteKit promotion awareness.3. [ok] Role state initialization order
getRole()is called beforegetStakeholderPrefix()in the DOMContentLoaded handler, which ensures sessionStorage is populated before the prefix check reads the role. Correct.4. [ok] Access control
isAllowedPath()correctly restricts stakeholders to assets bucket + their scoped prefix. Stakeholders navigating toindex.htmlare redirected. Delete buttons are hidden. Breadcrumbs are scoped.5. [ok] Acceptance criteria
index.html?role=admin6. [ok] CSS quality
Signin page styles follow the existing design token system. Role banner uses proper flex layout with text-overflow handling.
body[data-page="signin"]override removes bottom padding correctly.VERDICT: APPROVE with nits
Two nits (redundant static nav in index.html, unsanitized innerHTML). Neither blocks merge. All acceptance criteria from issue #4 are met.
PR #5 Review
DOMAIN REVIEW
Tech stack: Vanilla HTML/CSS/JS playground (no frameworks, no npm). Served via
python3 -m http.server. Mobile-first design targeting 390px. This is a prototype/playground, not a production app.Architecture assessment: The role state management pattern (sessionStorage + URL params) is well-structured for a playground. The
data-pagerouting viaDOMContentLoadedswitch is clean. Moving bottom nav rendering from static HTML into JS was the right call for role-aware nav. TheescapeHtml()utility is present and used in the right places for theprojectparameter. The westside HTML comment pattern (@route,@auth,@api,@interactivity,@gaps,@notes) is well-documented onsignin.html.Mobile/accessibility: 44px min tap targets are enforced on buttons, nav items, form inputs, and demo links.
aria-current="page"on active nav items.aria-labelon nav elements.aria-hidden="true"on decorative icons. No hover-dependent UI -- all interactions use:activestates.body[data-page="signin"]removes bottom padding correctly.env(safe-area-inset-bottom)for notch-safe spacing.max-width: 360pxon signin card keeps it contained at 390px.CSS quality: Uses design tokens consistently (no magic numbers for colors). Spacing uses rem units throughout. Role banner uses
text-overflow: ellipsisfor long project names. No horizontal scroll risk -- all containers usewidth: 100%withmax-widthconstraints.BLOCKERS
1. XSS: URL params
bucket,key,prefixparts, andfileNameare inserted into innerHTML without escapingThe PR correctly addresses
projectwithescapeHtml()(lines 362, 481, 484, 643, 704, 849 ofapp.js), but multiple other URL-derived values go intoinnerHTMLunescaped:bucket(fromgetUrlParam('bucket')) at lines 502, 514, 656, 709, 861, 876key(fromgetUrlParam('key')) at line 877fileName(derived fromkeyviagetFileName()) at lines 652, 664, 857, 869part(derived from splittingprefixURL param) at lines 491, 493, 508, 510, 715file.name(from File API) at line 772A crafted URL like
browse.html?bucket=<img src=x onerror=alert(1)>would execute arbitrary JS. The PR body explicitly notes the XSS nit from PR #2, andescapeHtml()exists and is used forproject-- it just needs to be applied to all URL-derived values rendered as text content.2. Missing access control on
preview.html,detail.html, andupload.htmlfor stakeholdersrenderObjectBrowser()(line 463) correctly callsisAllowedPath()to enforce prefix scoping. However:renderPreview()(line 613) has noisAllowedPath()check. A stakeholder can navigate directly topreview.html?bucket=assets&key=logos/pal-e-logo.png&role=stakeholderand view files outside their prefix.renderDetail()(line 809) has noisAllowedPath()check. Same bypass.initUpload()(line 687) has noisAllowedPath()check. A stakeholder could navigate toupload.html?bucket=postgres-wal&role=stakeholderand see an upload form for an unauthorized bucket.The issue requirement states "Stakeholder cannot navigate outside their scoped prefix." This is enforced on
browse.htmlbut not on the other three pages.NITS
Admin "Settings" nav item is actually a sign-out button (line 331). The nav label says "Settings" with a gear icon, but
onclick="signOut()"signs the user out. This is misleading -- either rename it to "Sign Out" or make it a real settings placeholder that includes a sign-out option.DRY: Breadcrumb rendering is duplicated across four functions (
renderObjectBrowser,renderPreview,renderDetail,initUpload). Each has a stakeholder vs. admin fork with nearly identical breadcrumb-building logic. A sharedbuildBreadcrumbs(bucket, keyOrPrefix, options)helper would reduce the ~120 lines of duplicated breadcrumb code to one call site.DRY: Role URL param propagation is repeated in every link-building block (
if (isStakeholder()) { params.set('role', ...); params.set('project', ...); }). A helper likeaddRoleParams(params)would centralize this.getStakeholderPrefix()has a subtle coupling issue (line 43-44): It callsgetRole()internally, which reads and caches theroleURL param. IfgetStakeholderPrefix()is called beforegetRole()on a page without aroleURL param but with aprefixparam, the prefix won't be stored. The DOMContentLoaded block (line 954-957) calls them in the right order, but it's fragile.rolevalue in href attributes is not URL-encoded (lines 323, 327, 499, 655, 708, 860):'?role=' + roleshould useencodeURIComponent(role)for correctness, though in practice the value is always 'admin' or 'stakeholder'.Admin nav browse link loses
roleparam (line 502):<a href="browse.html?bucket=${encodeURIComponent(bucket)}">${bucket}</a>in admin breadcrumbs does not propagaterole=admin, so clicking the bucket name in breadcrumbs would drop the admin context.renderBottomNav()callsgetRole()which can cause redundant sessionStorage writes on every page render sincegetRole()re-reads URL params each time it's called. Not a bug, but worth noting for when this moves to real auth.SOP COMPLIANCE
4-auth-flow-role-based-viewsmatches issue #4plan-minio-mobilereferencedPROCESS OBSERVATIONS
@gapsannotations insignin.htmlare excellent for SvelteKit promotion. They clearly document what needs to change for production (Keycloak OIDC, realm roles, prefix mapping).VERDICT: NOT APPROVED
Two blockers must be fixed:
escapeHtml()to all URL-derived values rendered as text content in innerHTML (bucket, key, fileName, part, file.name). The utility exists -- it just needs consistent application.isAllowedPath()checks torenderPreview(),renderDetail(), andinitUpload()to match the access control already implemented inrenderObjectBrowser().Blocker Fixes Pushed
Commit
d82a1a2addresses both blockers from the QA review.BLOCKER 1 -- XSS (fixed)
Applied
escapeHtml()to all URL-derived values inserted via innerHTML:bucket-- in admin breadcrumbs acrossrenderObjectBrowser(),renderPreview(),initUpload(),renderDetail(), and in the metadata table ondetail.htmlkey-- in the metadata table ondetail.htmlfileName(derived fromkeyviagetFileName()) -- in breadcrumb "current" spans acrossrenderPreview()andrenderDetail()part/parts[i](derived from splittingprefixorkey) -- in breadcrumb links and labels across all four page renderersfile.name(from File API) -- insimulateUpload()progress item renderingBLOCKER 2 -- Incomplete access control (fixed)
Added
isAllowedPath()/isAllowedKey()checks to the three unguarded page renderers:renderPreview()-- checksisAllowedKey(bucket, key)before rendering; redirects to scoped browse view on violationrenderDetail()-- checksisAllowedKey(bucket, key)before rendering; redirects to scoped browse view on violationinitUpload()-- checksisAllowedPath(bucket, prefix)before rendering; redirects to scoped browse view on violationAdded two helpers:
isAllowedKey(bucket, key)-- for pages that receive akeyparam instead ofprefix, checks if the key path starts with the stakeholder's allowed prefixredirectToScopedView()-- centralizes the redirect-to-scoped-browse logic used by all access control guardsPR #5 Re-Review
Re-review after two blockers were fixed. Verifying both fixes thoroughly.
BLOCKER #1 VERIFICATION: XSS (escapeHtml coverage)
Previous finding:
escapeHtml()was only applied toprojectparam. Other URL-derived values (bucket,key,fileName,part,file.name) were inserted into innerHTML unescaped.Current state: RESOLVED. The
escapeHtml()function is now applied consistently across all innerHTML insertions where URL-derived or user-supplied values appear in text content positions:renderRoleHeader()--escapeHtml(project)appliedrenderObjectBrowser()breadcrumbs --escapeHtml(project),escapeHtml(part),escapeHtml(bucket),escapeHtml(name)all appliedrenderObjectBrowser()thumbnail grid --escapeHtml(getFileName(obj.key))appliedrenderPreview()breadcrumbs --escapeHtml(project),escapeHtml(bucket),escapeHtml(parts[i]),escapeHtml(fileName)all appliedrenderDetail()breadcrumbs and metadata table --escapeHtml(project),escapeHtml(bucket),escapeHtml(key)all appliedinitUpload()breadcrumbs --escapeHtml(project),escapeHtml(bucket),escapeHtml(part)all appliedsimulateUpload()--escapeHtml(file.name)appliedURL parameters embedded in
hrefattribute values useencodeURIComponent()consistently, which is the correct encoding for that context.One minor gap (nit, not a blocker): In
renderBucketList(),${bucket.name}is inserted into innerHTML withoutescapeHtml(). This is sourced from the hardcodedMOCK_BUCKETSconstant, not from URL params, so it is not an XSS vector in the current prototype. When this code is promoted to SvelteKit with real data, this would need attention.BLOCKER #2 VERIFICATION: Incomplete Access Control
Previous finding:
isAllowedPath()was only enforced inrenderObjectBrowser(). TherenderPreview(),renderDetail(), andinitUpload()functions had no access control, allowing stakeholders to directly navigate to files outside their scoped prefix.Current state: RESOLVED. Access control is now enforced at the top of every page renderer before any content is displayed:
renderBucketList()isStakeholder()check redirects to scoped browse viewbrowse.htmlwith scoped paramsrenderObjectBrowser()isStakeholder() && !isAllowedPath(bucket, prefix)redirectToScopedView()renderPreview()isStakeholder() && !isAllowedKey(bucket, key)redirectToScopedView()renderDetail()isStakeholder() && !isAllowedKey(bucket, key)redirectToScopedView()initUpload()isStakeholder() && !isAllowedPath(bucket, prefix)redirectToScopedView()The
isAllowedKey()function is a proper complement toisAllowedPath()-- it validates that a full object key starts with the stakeholder's allowed prefix, appropriate for preview/detail views that operate on keys rather than prefixes.The
redirectToScopedView()function correctly builds a URL withencodeURIComponent()on all parameters and redirects to the stakeholder's allowed browse path.Unauthenticated users (no role set) are redirected to
signin.htmlacross all renderers.DOMAIN REVIEW
Tech stack: Vanilla HTML/CSS/JS playground (pre-SvelteKit prototype).
Accessibility:
aria-label="Main navigation"andaria-current="page"for active state -- goodrole="button",tabindex="0", andaria-label-- goodaria-label="Breadcrumb"-- goodCSS:
Architecture observations for SvelteKit promotion:
@route,@auth,@api,@interactivity,@gaps,@notes) insignin.htmlare a useful pattern for migration planningMOCK_BUCKETS,MOCK_OBJECTS) cleanly separable from rendering logicBLOCKERS
None. Both previous blockers are resolved.
NITS
renderBucketList()unescapedbucket.name: The${bucket.name}value in the bucket grid innerHTML is not wrapped inescapeHtml(). Currently safe becauseMOCK_BUCKETSis hardcoded, but should be escaped when real data is introduced. (app.js, renderBucketList function)renderBottomNav()role in href: The admin branch doesindex.html${role ? '?role=' + role : ''}whereroleis concatenated directly. The value is constrained to 'admin'/'stakeholder' from the demo links, butencodeURIComponent(role)would be more defensive. (app.js, renderBottomNav function)contentTypein renderDetail unescaped: The metadata table renders${obj.contentType}withoutescapeHtml(). This comes fromMOCK_OBJECTSdata, not URL params, so not currently exploitable, but worth noting for promotion. (app.js, renderDetail function)SOP COMPLIANCE
4-auth-flow-role-based-viewsreferences issue #4)plan-minio-mobile)PROCESS OBSERVATIONS
This is a playground prototype (pre-SvelteKit), so the absence of automated tests is expected -- there is no test framework to run against vanilla HTML/JS files. The Test Plan section in the PR body provides manual verification steps, which is appropriate for this stage. When promoted to SvelteKit, the access control logic and escapeHtml patterns should be covered by unit tests.
The two-round review cycle worked correctly: blockers were identified, fixed, and verified. The fixes are thorough --
escapeHtml()coverage is comprehensive and access control is enforced at every entry point.VERDICT: APPROVED