Modernize module iframes while preserving dashboard flow (#914) - #920
abhinav-phi wants to merge 1 commit into
Conversation
Replace deprecated frameborder string injection with DOM-created sandboxed frames. Adds title, loading lazy, referrerpolicy same-origin, and same-origin sandbox that blocks top navigation but keeps scripts, forms, modals, popups and downloads for training content. Adds same-origin src validation and fixes getStarted double-html handling. No Java or session logic changed.
|
Hi, this update covers the bounded iframe work for #914. I replaced the deprecated Static checks and the Node validation for the new helper pass in my environment. Maven tests and full Docker runtime checks are still pending, so review plus local verification are needed before merge. Thanks. Related issue: #914 |
|
Hi SeanDuggan, this PR is ready for review whenever you have a moment. It implements the bounded iframe modernization for #914: sandboxed same-origin frames created through DOM APIs with title, lazy loading, and a same-origin referrer policy, plus source validation and the Two limitations to flag upfront: the Maven tests and a full Docker runtime check are still pending on my side, and I would appreciate your confirmation that keeping sandboxed frames matches the intended scope. Thanks. |
|
Hi Abinhav-Phi, This one will take me a while to test. Once testing is done, I'll follow up with comments or close depending on results. Thanks, |
Closes #914.
Scope
Issue #914 asks for replacing the iframes on the web application with a modern alternative. The module views are full documents with their own scripts and relative paths, while result submission stays in the dashboard. Converting every module page into dashboard-compatible fragments would be a full frontend rewrite, so this PR takes a deliberately bounded approach: it keeps same-origin frames for lesson and challenge content, but modernizes how those frames are created and secured.
Changes
In
src/main/webapp/index.jsp, I addedisSafeModuleSrcandcreateModuleFramehelpers and replaced the deprecatedframeborder='no'string-injected iframes in all three module loaders (the open/tourney challenge loader, the open/tourney lesson loader, and the CTF-mode lesson loader) with DOM-created frames. The new frames use atitleattribute,loading="lazy",referrerpolicy="same-origin", and a sandbox ofallow-same-origin allow-scripts allow-forms allow-modals allow-popups allow-popups-to-escape-sandbox allow-downloads. The sandbox blocks top-level navigation so module content cannot break out of the dashboard, while preserving the scripts, forms, alerts, externaltarget="_blank"links, and downloads the existing lessons rely on. ThegetModuleresponse is validated as a same-origin relative address before loading, and the deprecated jQuery.load()calls were replaced with.on("load"), removing the duplicateappendToafterhtml().In
src/main/webapp/getStarted.jsp, I fixed the doublehtml()handling that was clearing the welcome frame, and thereadyToPlay.jspframe is now created with the same modern attributes through DOM APIs. Insrc/main/webapp/css/theCss.css, I addedborder: 0to.levelIframeto replace the deprecatedframeborderattribute.No Java, session, CSRF, or
getModule/solutionSubmitlogic was changed, and the training servlets underservlets/module/lessonandservlets/module/challengeare untouched.Verification
I verified statically that zero
frameborderiframe attributes and zero string-concatenated<iframecreations remain in code (one mention of the word survives only inside a code comment), and that the helper plus sandbox attributes are present in bothindex.jspandgetStarted.jsp. The JavaScript helper passesnode --check, and its same-origin validation correctly accepts relative module paths while rejectingjavascript:,data:, absolute, and protocol-relative URLs. The CSS change is confirmed present.Maven is not installed in this environment, so
mvn testandmvn spotless:checkwere not run. No Java files were changed, but a full build and Docker runtime verification are still pending.Remaining work and limitations
This still needs maintainer confirmation that keeping sandboxed frames, rather than converting modules into dashboard fragments, matches the intended scope for #914. It also needs CI plus a local Docker run (login, open a lesson and a challenge, submit a result, and check the cheat-sheet toggle) before merge. No new automated tests were added because there is no existing JavaScript or JSP test harness covering these views, so manual regression is required.