Address reported security issues in the testresults module#654
Open
vagisha wants to merge 5 commits into
Open
Conversation
* failureDetail.jsp: HTML-encode the failure language and stack trace with LABKEY.Utils.encodeHtml before building the row markup, rather than concatenating them straight into innerHTML. * runDetail.jsp: render the run log/xml popup via a <pre> textContent node instead of document.write of concatenated HTML. * trainingdata.jsp: render the email preview in a sandboxed iframe (scripts disabled, opaque origin) instead of document.write of the raw email HTML. All three display stored run/failure data that originates from the unauthenticated SkylineNightly results upload (PostAction), so a crafted post could otherwise store script that runs in an admin's browser. Co-Authored-By: Claude <noreply@anthropic.com>
…urces * ChangeBoundaries.execute: the GlobalSettings update transaction was opened with a bare assignment and committed with no try/finally, so it could leak if an exception was thrown before commit. Wrapped it in try-with-resources, like every other transaction in this controller. * PostAction / PostErrorFilesAction: documented that the stack trace returned in the response is intentional, so SkylineTester can diagnose posting failures from the client without server log access. Noted by the security review; kept by design. Co-Authored-By: Claude <noreply@anthropic.com>
…t includes the exception, per the logging convention. TestResultsController used its existing _log; RunDetail, TestResultsModule, and TestResultsWebPart got a static LOG via LogHelper.getLogger. * ChangeBoundaries.execute: parameterized the GlobalSettings INSERT with bind parameters (VALUES (?, ?)) instead of concatenating the warning/error integer values into the SQL string. Co-Authored-By: Claude <noreply@anthropic.com>
…g-user name, problem-table computer name, and test name. * trainingdata.jsp: the email-preview iframe used height:100%, which collapses to zero against the popup body (no defined height) and showed a blank popup; switched to 100vh so the preview renders. * TestResultsTest: added testViewLogPopup, which opens the run-log popup and asserts the stored log is displayed. Co-Authored-By: Claude <noreply@anthropic.com>
* TestResultsController SaveXML: parameterized the two filename log statements * ChangeBoundaries.execute: removed the unused GlobalSettings local * TestResultsModule: moved the LogHelper import to its correct position. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale
A security review of the testresults module identified several issues, addressed here. (The cross-container access findings were already fixed on the 26.3 release branch and are not part of this PR.)
Changes
*Stored XSS (High) - result views and email preview
<pre>textContent node instead of document.write of concatenated HTML.Transaction and SQL handling (Medium)
Daily email output encoding (Low)
Logging hygiene (Low)
Tests
Not in scope (by decision)
Co-Authored-By: Claude noreply@anthropic.com