Skip to content

Store enclosure metadata without fetching remote URLs - #175

Open
snoopdave wants to merge 1 commit into
masterfrom
enclosure-metadata-entry
Open

Store enclosure metadata without fetching remote URLs#175
snoopdave wants to merge 1 commit into
masterfrom
enclosure-metadata-entry

Conversation

@snoopdave

Copy link
Copy Markdown
Contributor

Summary:

  • collect enclosure media type and byte length alongside its URL
  • validate HTTP(S) enclosure metadata locally before saving
  • populate enclosure metadata from uploaded media files
  • remove the legacy remote metadata lookup classes

Testing:

  • mvn -pl app -Dtest=EnclosureMetadataTest test
  • mvn -V -ntp install (Temurin JDK 11.0.31)

@mraible mraible left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • MediacastUtil stored con.getContentType() verbatim, so existing entries can carry audio/mpeg; charset=utf-8; the new MEDIA_TYPE regex has no parameter support, so every save of such an entry fails, even a typo fix in the body.
  • UrlValidator rejects hosts whose TLD isn't in its IANA list (.local, .lan, .internal, .test), and ALLOW_LOCAL_URLS only 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 &, and feeds.vm emits type="$mc_type" without escapeXML in both the RSS and Atom macros, so a "valid" type can break the whole feed.
  • MediaFileAddSuccess.jsp now inlines the upload's declared contentType into a single-quoted JS literal in onchange; escapeHtml4 doesn't escape '.
  • The early return INPUT skips the entryAdd status reset at the end of save(), 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");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

@mraible mraible Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}"/>',

@mraible mraible Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants