Store enclosure metadata without fetching remote URLs - #175
Conversation
mraible
left a comment
There was a problem hiding this comment.
Dropping the server-side HEAD fetch is the right call for the SSRF, and pre-filling the metadata from uploaded media is a nice touch. What blocks it for me is that validation went from advisory to fatal, which locks some existing entries and some installs out of saving at all (inline). Summary:
MediacastUtilstoredcon.getContentType()verbatim, so existing entries can carryaudio/mpeg; charset=utf-8; the newMEDIA_TYPEregex has no parameter support, so every save of such an entry fails, even a typo fix in the body.UrlValidatorrejects hosts whose TLD isn't in its IANA list (.local,.lan,.internal,.test), andALLOW_LOCAL_URLSonly helps single-label hosts. The uploaded-media permalink is built from the site URL, so an intranet install can never attach uploaded media as an enclosure.- The regex admits
&, andfeeds.vmemitstype="$mc_type"withoutescapeXMLin both the RSS and Atom macros, so a "valid" type can break the whole feed. MediaFileAddSuccess.jspnow inlines the upload's declaredcontentTypeinto a single-quoted JS literal inonchange;escapeHtml4doesn't escape'.- The early
return INPUTskips theentryAddstatus reset at the end ofsave(), so a failed new-entry publish re-renders as "Published".
I'd suggest: validate on the way in (form) but treat a bad legacy value as "drop the enclosure and warn" rather than refusing the save; split the message so it says which field is wrong; and either escape $mc_type in feeds.vm or tighten the regex to [A-Za-z0-9!#$%^*+.\-_]+/[A-Za-z0-9!#$%^*+.\-_]+ (no &, ', `, |).
| getBean().getEnclosureType(), | ||
| getBean().getEnclosureLength()); | ||
| } catch (IllegalArgumentException e) { | ||
| addError("weblogEdit.enclosureMetadataInvalid"); |
There was a problem hiding this comment.
This is fatal where the old flow was advisory (addMessage and continue), and existing entries can't pass it: the removed MediacastUtil stored con.getContentType() verbatim, so att_mediacast_type may be audio/mpeg; charset=utf-8 or video/mp4;codecs=avc1, which MEDIA_TYPE rejects. The author then can't save any change to that entry until they notice and hand-edit the type. Either accept parameters in the regex (and strip them), or treat an invalid legacy value as "clear the enclosure and warn" instead of refusing the save. Also, one generic message for three fields: a blank Length (new field, previously auto-filled) produces the same text as a bad URL.
| */ | ||
| public final class EnclosureMetadata { | ||
|
|
||
| private static final UrlValidator URL_VALIDATOR = new UrlValidator( |
There was a problem hiding this comment.
UrlValidator rejects any host whose TLD isn't in its IANA list (blog.internal, roller.local, blog.lan, .test), underscores, and IDN hosts; ALLOW_LOCAL_URLS only admits single-label hosts like localhost. Since MediaFileAddSuccess pre-fills the enclosure URL from the site's own absolute URL, an intranet install can never attach uploaded media as an enclosure, and existing entries with such URLs can no longer be re-saved. java.net.URI with a scheme check (http / https, non-empty host) is enough here; the point is no longer fetching it.
| private static final UrlValidator URL_VALIDATOR = new UrlValidator( | ||
| new String[] {"http", "https"}, UrlValidator.ALLOW_LOCAL_URLS); | ||
|
|
||
| private static final Pattern MEDIA_TYPE = Pattern.compile( |
There was a problem hiding this comment.
This admits & (and ', `, |), but feeds.vm lines 52 and 80 emit type="$mc_type" without escapeXML, so audio/mp&eg passes validation and turns the whole RSS and Atom feed into malformed XML. Either escape it in feeds.vm or drop those characters from the character class (real media types never use them).
| <input type="radio" name="enclosure" | ||
| onchange="setEnclosure('<s:property value="%{#newFile.permalink}"/>')"/> | ||
| onchange="setEnclosure('<s:property value="%{#newFile.permalink}"/>', | ||
| '<s:property value="%{#newFile.contentType}"/>', |
There was a problem hiding this comment.
#newFile.contentType comes from the upload's declared Content-Type (MediaFileAdd only overrides null / octet-stream), and <s:property> HTML-escapes without touching ', so a part declared as audio/x'; alert(1)// runs in the uploader's session, and a stray quote just kills the handler so the radio does nothing. Use escapeJavaScript="true" escapeHtml="false", or better, put the three values in data- attributes and read them in the handler. (#174 changes how contentType is derived, which narrows this, but the JSP should still not inline it unescaped.)
| getBean().getEnclosureLength()); | ||
| } catch (IllegalArgumentException e) { | ||
| addError("weblogEdit.enclosureMetadataInvalid"); | ||
| return INPUT; |
There was a problem hiding this comment.
This returns before the if ("entryAdd".equals(actionName)) getBean().setStatus(null) reset at the end of the method (line 309), which every other failed save on a new entry goes through. publish() has already stamped PUBLISHED on the bean, so the form re-renders with the green "Published (Last updated: )" badge and an empty date for an entry that was never written, and the hidden bean.status carries PUBLISHED into the next submit.
| } | ||
| if (!MEDIA_TYPE.matcher(normalizedType).matches()) { | ||
| throw new IllegalArgumentException("Enclosure type must be a valid media type"); | ||
| } |
There was a problem hiding this comment.
Nit: no upper bound; Long.MAX_VALUE is accepted and the feed advertises an 8 EiB enclosure. The form caps the field at 20 characters, so a sanity ceiling here would match.
| weblogEdit.mediaCastUrlMalformed=The enclosure URL was malformed. | ||
| weblogEdit.mediaCastResponseError=The enclosure server returned an error. Do you have the right URL? | ||
| weblogEdit.mediaCastLacksContentTypeOrLength=Unable to use enclosure URL. Server provided no content type or no length. | ||
| weblogEdit.enclosureURL.tooltip=Absolute HTTP or HTTPS URL to embed within the RSS & Atom feeds for this blog entry. |
There was a problem hiding this comment.
Nit: the four removed weblogEdit.mediaCast* keys are still in the _de / _es / _fr / _ja / _ko / _ru / _zh_CN bundles, and the ja / zh_CN tooltips still describe the old "podcast URL" semantics rather than the HTTP(S)-only requirement that now produces the error.
Summary:
Testing: