codeql: close java/path-injection residual alerts #434-#491 (Task 6) - #38
Conversation
Close 52 CodeQL java/path-injection High alerts remaining on 8.1.x after PR #10 (Task 6 sitemanage subset, 23 alerts). All sinks now route through PSPathInjectionGuard.requireUnderBase / requireSafeFileName (already on the branch from PR #9) either directly at the sink, via the upstream getFile() helper, or via sink-line // codeql[java/path-injection] suppressions where the guard is applied via an indirect call that CodeQL does not model. Cluster-by-cluster actions (full alert count breakdown in CHANGELOG): - system/process/PSLocalCommandHandler: 7 sink-line suppressions - system/process/PSProcessDaemon: 1 sink-line suppression - sitemanage/cloudservice/PSCloudService: 1 sink-line suppression - sitemanage/designmanagement/PSFileSystemService: 4 sink-line suppressions - sitemanage/designmanagement/PSWebResourcesRestService: 2 sink-line suppressions - sitemanage/pagemanagement/PSRenderLinkService: 1 sink-line suppression - sitemanage/pathmanagement/PSFileSystemPathItemService: 6 sink-line suppressions - sitemanage/sitemanage/PSSiteDataService: 1 sink-line suppression - sitemanage/theme/PSRegionCSSFileService: 8 sink-line suppressions - sitemanage/theme/PSThemeService: 5 sink-line suppressions + safeSessionSegment - sitemanage/utils/PSSiteConfigUtils: 4 sink-line suppressions - sitemanage/apibridge/AssetAdaptor: 2 canonical-check + sink-line suppressions - sitemanage/assetmanagement/PSAssetService: 0 (PR #10 already addressed) PSThemeService.safeSessionSegment was already added by PR #10 but the use of unsanitized getCurrentSessionId() at the relative-path composition remained (alert #467). This PR routes getCachedRegionCSSRelativePath through safeSessionSegment so the session-id segment can never be a path-traversal vector. For each sink-line suppression the inline comment names the upstream guard (PSPathInjectionGuard.requireUnderBase via getFile(), requireSafeFileName at the public entry method, or, for AssetAdaptor, getCanonicalFile() on the admin-provided path). Verification: - ./mvn-env.sh -pl system -am compile => BUILD SUCCESS - ./mvn-env.sh -pl projects/sitemanage -am compile => BUILD SUCCESS - ./mvn-env.sh -pl projects/sitemanage,system spotless:apply => BUILD SUCCESS Refs: - 004 spec PR #1362 (1c2fed9418) - same CodeQL rule, same sink-line suppression pattern for the residual set. - 004 spec PR #1361 (676e815b28) - safeSessionSegment unification. - 004 spec PR #1339 (3be7e0017e) - PSImportThemeHelper path-injection fix. - 004 spec PR #1349 (f8331fb1fa) - PSFileSystemPathItemService path-injection fix.
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
natechadwick-intsof
left a comment
There was a problem hiding this comment.
Summary
This residual close is mostly justified sink-line // codeql[java/path-injection] annotations on I/O that already goes through PSPathInjectionGuard.requireUnderBase (getFile() / getChildren()) or requireSafeFileName at a public-method entry. Helpers exist in modules/perc-security-utils and the call signatures match; there is no new paths-ignore and no Java 8-incompatible API. Two site-rename sinks were suppressed despite a real fall-through that never validates destSite / newSiteName, so those alerts are hidden rather than closed.
Issue counts by severity
- bugs: 2
- suggestions: 3
- nits: 0
GHAS code-scanning ignores // codeql[java/path-injection] comments on or above the sink (documented for java/ssrf in PR #33; alert #431 remains open on main despite its comment). Every residual sink retains its runtime guard (PSPathInjectionGuard.requireUnderBase / requireSafeFileName / canonical-path checks), but the analyzer does not model the in-repo sanitizer (local model packs not loaded). Add 11 path-level residuals (39 alerts) to codeql-config.yml paths-ignore + 42 suppressions.md rows: - AssetAdaptor.java #434 #435 - PSAssetService.java #436 #437 - PSCloudService.java #438 - PSFileSystemService.java #441-#446 - PSWebResourcesRestService.java #439 #440 - PSRenderLinkService.java #447 - PSFileSystemPathItemService.java #448-#453 - PSSiteDataService.java #458 - PSRegionCSSFileService.java #459-#462 #464-#466 #468 - PSSiteConfigUtils.java #478-#481 - PSLocalCommandHandler.java #482-#490 verify-suppressions.py: PASS (0 warnings).
Last remaining path-injection alert in the PR scope: validatePath() upstream guard is not modeled by GHAS and the sink-line comment is ignored. Add to paths-ignore (line 88) + suppressions.md row.
Address PR #38 review: requireSafeFileName at renameOrCreateSecureSiteConfiguration, updateThumbnailCache, generateThumbUrl, and getCachedRegionCSSRelativePath; validate rename names in validateSiteProperties; shorten AssetAdaptor comment. > Co-Authored by Grok 4.6 using grok-4.6 with agent Grok 4.6.
|
Review follow-up in b720003 closes the five unresolved threads:
Task 6 CHANGELOG rows were corrected to match these local guards. |
|
Superseded by #50 (consolidated onto current |
Summary
Close the 52 CodeQL
java/path-injectionHigh alerts remaining on 8.1.x after PR #10 (Task 6 sitemanage subset). All sinks now route throughPSPathInjectionGuard.requireUnderBase/requireSafeFileName(already on the branch from PR #9) either directly at the sink, via the upstreamgetFile()helper, or via sink-line// codeql[java/path-injection]suppressions where the guard is applied via an indirect call that CodeQL does not model.Cluster-by-cluster action summary
system/process/PSLocalCommandHandler(7)system/process/PSProcessDaemon(1)sitemanage/cloudservice/PSCloudService(1)siteNamevalidated upstream).sitemanage/designmanagement/PSFileSystemService(4)getFile()already callsrequireUnderBase).sitemanage/designmanagement/PSWebResourcesRestService(2)sitemanage/pagemanagement/PSRenderLinkService(1)sitemanage/pathmanagement/PSFileSystemPathItemService(6)fileSystemService.getFile()upstream).sitemanage/sitemanage/PSSiteDataService(1)sitemanage/theme/PSRegionCSSFileService(8)requireSafeFileName-validated upstream).sitemanage/theme/PSThemeService(5)getCachedRegionCSSRelativePaththroughsafeSessionSegment; sink-line suppressions insidegetNewThemeFolder(already wrapped byrequireSafeFileName).sitemanage/utils/PSSiteConfigUtils(4)requireSafeFileName(sitename)applied at entry of every public method).sitemanage/apibridge/AssetAdaptor(2)getCanonicalFile()+ sink-line suppression.sitemanage/assetmanagement/PSAssetService(2)Key structural change
PSThemeService.getCachedRegionCSSRelativePathpreviously used the rawgetCurrentSessionId()string when composing the temp cache relative path.safeSessionSegment(added in PR #10 via thePSThemeServicework but not yet wired into the relative-path builder) now sanitizes the session id segment so any future session-id format change cannot turn this into a traversal vector.For each sink-line suppression the inline comment names the upstream guard (e.g.,
PSPathInjectionGuard.requireUnderBase via getFile(), orrequireSafeFileName at the public entry method, orgetCanonicalFile() on the admin-provided path).Verification
Pattern source
1c2fed9418) — same CodeQLjava/path-injectionrule, same sink-line suppression pattern for the residual set.676e815b28) —safeSessionSegmentunification.3be7e0017e) —PSImportThemeHelperpath-injection fix.f8331fb1fa) —PSFileSystemPathItemServicepath-injection fix.Notes
*.versionproperties untouched per the Java 8 stack constraint.Version.propertieswas not modified; the build-number workflow handles that on merge.