Normalize frontpage directory parameters before rendering the bundled theme - #169
Normalize frontpage directory parameters before rendering the bundled theme#169snoopdave wants to merge 1 commit into
Conversation
… theme The bundled frontpage directory filters its listing by a letter parameter. It now accepts that parameter only when it matches one of the directory's own A-Z keys, falling back to the full listing otherwise (as a missing parameter already did), and HTML-escapes it in the heading. The sibling directory template resolves its weblog handle before use and builds the back-link from the resolved weblog rather than the request parameter. Adds FrontpageDirectoryRenderingTest and tightens the weblog letter-map test to assert the complete A-Z key set the template now depends on. Claude-Session: https://claude.ai/code/session_01A1fhY1E2PCFU6UAPXu2WtV
mraible
left a comment
mraible
left a comment
There was a problem hiding this comment.
The letter gating and building the back-link from the resolved handle are good. Two things keep the weblog parameter from being "validated before use" as the title says (inline), plus test and cleanup nits. Happy to approve once the first two are addressed.
| #set($profileWeblog = false) | ||
| #set($requestedHandle = $model.getRequestParameter("weblog")) | ||
| #if($requestedHandle) | ||
| #set($profileWeblog = $site.getWeblog($requestedHandle)) |
There was a problem hiding this comment.
$site.getWeblog() still receives the raw parameter. JPAWeblogManagerImpl.getWeblogByHandle throws WebloggerException("Invalid handle: '...'") for anything outside [A-Za-z0-9_], and SiteModel.getWeblog logs that at ERROR with a stack trace, so an anonymous loop over /page/directory?weblog=<junk> fills the log with attacker-controlled text (CR/LF included). A cheap pre-check such as #if($requestedHandle && $requestedHandle.matches("[A-Za-z0-9_]+")) keeps garbage from reaching the manager.
| #if($requestedHandle) | ||
| #set($profileWeblog = $site.getWeblog($requestedHandle)) | ||
| #end | ||
| #if($profileWeblog) |
There was a problem hiding this comment.
Handles may start with a digit (username.allowedChars defaults to A-Za-z0-9), and getWeblogsByLetter handled ?letter=2 before. Now 2 isn't a key in the A-Z map, so "Back to blog directory" from such a profile lands on the unfiltered list. Either accept the resolved handle's first character in _blogdirectory.vm (it's trusted) or omit the letter param when it isn't A-Z.
| ## as a missing parameter does. | ||
| #set($requestedLetter = $model.getRequestParameter("letter")) | ||
| #if($requestedLetter && $requestedLetter.length() == 1) | ||
| #set($candidateLetter = $requestedLetter.toUpperCase()) |
There was a problem hiding this comment.
Minor: toUpperCase() uses the JVM default locale, so on a Turkish-locale server i becomes İ and ?letter=i is rejected while ?letter=I works. A Locale.ROOT upper-case helper on $utils would avoid it.
| ## Accept only a known A-Z key; otherwise render the full listing, exactly | ||
| ## as a missing parameter does. | ||
| #set($requestedLetter = $model.getRequestParameter("letter")) | ||
| #if($requestedLetter && $requestedLetter.length() == 1) |
There was a problem hiding this comment.
Nit: the length() == 1 check and $candidateLetter collapse to #if($requestedLetter && $weblogLetterMap.containsKey($requestedLetter.toUpperCase())), since every key is one character. Keep the null guard, TreeMap.containsKey(null) throws.
| #set($profileWeblog = $site.getWeblog($requestedHandle)) | ||
| #end | ||
| #if($profileWeblog) | ||
| <a href="?letter=$utils.escapeHTML($utils.left($profileWeblog.handle,1))">Back to blog directory</a> |
There was a problem hiding this comment.
Nit: escapeHTML can't change anything here, $profileWeblog.handle already passed the [A-Za-z0-9_] check in getWeblogByHandle.
| #set($profileWeblog = $site.getWeblog($handle)) | ||
| ## Render the profile only for a weblog that exists, and build | ||
| ## the back-link from the resolved weblog's own handle. | ||
| #set($profileWeblog = false) |
There was a problem hiding this comment.
Nit: nothing sets $profileWeblog before this and Velocity 2.4 assigns null from #set, so the initialiser is a no-op. If it's kept for the ROL-689 precedent in weblog.vm, a comment saying so would help.
| * parameter, and escaped at output. | ||
| */ | ||
| @Test | ||
| public void directoryTemplateValidatesTheWeblogParameter() throws Exception { |
There was a problem hiding this comment.
This asserts source substrings of directory.vm, so a whitespace change breaks it while a re-introduced raw parameter under another variable name passes. #includeTemplate is a plain velocimacro in WEB-INF/velocity/weblog.vm, so the test can stub it inline (#macro(includeTemplate $w $p)#end before #parse('directory.vm')) and assert the rendered output: no profile for an unknown handle, back-link built from the resolved handle for a known one.
| props.setProperty("resource.loaders", "file"); | ||
| props.setProperty("resource.loader.file.class", | ||
| "org.apache.velocity.runtime.resource.loader.FileResourceLoader"); | ||
| props.setProperty("resource.loader.file.path", THEME_DIR); |
There was a problem hiding this comment.
app/pom.xml already copies src/main/webapp/themes/** onto the test classpath (that's how themes.dir in roller-custom.properties works), so a ClasspathResourceLoader rooted at themes/frontpage/ drops the working-directory dependence.
| } | ||
| } | ||
|
|
||
| public static class StubUtils { |
There was a problem hiding this comment.
Nit: UtilitiesModel has a no-arg constructor and left / escapeHTML are stateless, so ctx.put("utils", new UtilitiesModel()) tests the real escapeHtml4 instead of a four-replace stand-in.
2 participants
The bundled frontpage theme's blog-directory page filters its listing by a
letterrequest parameter and builds a back-link from aweblogparameter.This change validates and normalizes both before use and escapes them at output.
What changed
to be a key in the business layer's weblog-letter map.
without redirecting, erroring, or echoing the rejected value.
weblogparameter to an existing weblog and build the back-linkfrom the resolved handle, escaped at output.
Tests
the same group.
full listing and do not appear in the response, raw or encoded.