You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@roller.apache.org by "snoopdave (via GitHub)" <gi...@apache.org> on 2026/09/14 23:07:16 UTC
[PR] Normalize links in HTML subset formatting [roller]
snoopdave opened a new pull request, #190:
URL: https://github.com/apache/roller/pull/190
Normalize supported links to a consistent href format while preserving comment text and basic formatting.
Adds coverage for quoted and unquoted links, query strings, whitespace, entity text, and plain-text comments.
Validation: `mvn -V -ntp install` on JDK 25 — 307 tests, no failures or errors, one skipped.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@roller.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Normalize links in HTML subset formatting [roller]
Posted by "snoopdave (via GitHub)" <gi...@apache.org>.
snoopdave commented on code in PR #190:
URL: https://github.com/apache/roller/pull/190#discussion_r4040981809
##########
app/src/main/java/org/apache/roller/weblogger/util/Utilities.java:
##########
@@ -80,7 +80,12 @@ public class Utilities {
private static final Pattern CLOSING_A_TAG_PATTERN = Pattern.compile(
"</a>", Pattern.CASE_INSENSITIVE);
private static final Pattern OPENING_A_TAG_PATTERN = Pattern.compile(
- "<a href=.*?>", Pattern.CASE_INSENSITIVE);
+ "<a\\s+href\\s*=.*?>", Pattern.CASE_INSENSITIVE | Pattern.DOTALL);
+ private static final Pattern A_HREF_PATTERN = Pattern.compile(
+ "<a\\s+href\\s*=\\s*(?:\"([^\"]*)\"|'([^']*)'|([^\\s\"'=<>`]+))\\s*>",
Review Comment:
🤖Claude: fixed by allowing `=` in unquoted href values. `A_HREF_PATTERN` now runs against the tag's interior (the text between the escaped `<a` and `>`) with `find()` instead of against the whole tag with `matches()`, so the escaped `>` terminator is no longer inside the search space and cannot be swallowed by a greedy unquoted value. The class is now `[^\s"'<>`]+`, terminating on whitespace or end of attributes. `<a href=https://ex.com/?a=1>` and `<a href=https://ex.com/?a=1&b=2>` both render as links again; added assertions for both in `testHtmlSubsetLinkFormats`.
##########
app/src/main/java/org/apache/roller/weblogger/util/Utilities.java:
##########
@@ -80,7 +80,12 @@ public class Utilities {
private static final Pattern CLOSING_A_TAG_PATTERN = Pattern.compile(
"</a>", Pattern.CASE_INSENSITIVE);
private static final Pattern OPENING_A_TAG_PATTERN = Pattern.compile(
- "<a href=.*?>", Pattern.CASE_INSENSITIVE);
+ "<a\\s+href\\s*=.*?>", Pattern.CASE_INSENSITIVE | Pattern.DOTALL);
+ private static final Pattern A_HREF_PATTERN = Pattern.compile(
+ "<a\\s+href\\s*=\\s*(?:\"([^\"]*)\"|'([^']*)'|([^\\s\"'=<>`]+))\\s*>",
+ Pattern.CASE_INSENSITIVE);
Review Comment:
🤖Claude: fixed by matching the href as an attribute rather than requiring the whole tag to be exactly `<a href=X>`. `A_HREF_PATTERN` is now `(?:^|\s)href\s*=\s*(...)` applied with `find()` to the tag's attribute text, so attributes before and after the href are tolerated and discarded. All three cases render as links again, and `href` no longer has to come first:
```
<a href="https://example.com/" target="_blank">x</a> -> <a href="https://example.com/">x</a>
<a href='https://example.com/' title='site'>x</a> -> <a href="https://example.com/">x</a>
<a class="external" href="https://example.com/">x</a> -> <a href="https://example.com/">x</a>
```
This stays safe because the output anchor is still reconstructed from the validated href and never echoes the input tag — `<a href="https://example.com/" onclick="void(0)">` yields `<a href="https://example.com/">` with the handler dropped, and `javascript:`/`data:` hrefs still degrade to `<a>`. New test `testHtmlSubsetLinkKeepsHrefAmongOtherAttributes` covers these; the `title` case in `testHtmlSubsetLinkSubset` was re-pointed at an `ftp://` href so it still asserts scheme rejection rather than attribute rejection.
##########
app/src/main/java/org/apache/roller/weblogger/util/Utilities.java:
##########
@@ -975,23 +980,39 @@ public static String transformToHTMLSubset(String s) {
s = replace(s, CLOSING_LI_TAG_PATTERN, "</li>");
s = replace(s, QUOTE_PATTERN, "\"");
- // HTTP links
+ // Normalize supported links while retaining the surrounding text.
s = replace(s, CLOSING_A_TAG_PATTERN, "</a>");
Review Comment:
🤖Claude: fixed by broadening `OPENING_A_TAG_PATTERN` to `<a(?:\s[\s\S]*?)?>` so it matches *every* escaped opening anchor, not just ones starting `<a href=`. Each one now goes through the same reconstruction, so an anchor whose href is missing or unusable becomes `<a>` and the markup stays balanced:
```
<a>x</a> -> <a>x</a>
<a name="anchor">x</a> -> <a>x</a>
<a class="c" href="https://x/">y</a> -> <a href="https://x/">y</a>
```
The pattern requires whitespace or the closing delimiter after `a`, so sibling tags are unaffected — `<abbr>` still passes through escaped. Covered by new test `testHtmlSubsetBalancesUnsupportedAnchors`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@roller.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Normalize links in HTML subset formatting [roller]
Posted by "snoopdave (via GitHub)" <gi...@apache.org>.
snoopdave commented on code in PR #190:
URL: https://github.com/apache/roller/pull/190#discussion_r4040899150
##########
app/src/main/java/org/apache/roller/weblogger/util/Utilities.java:
##########
@@ -80,7 +80,12 @@ public class Utilities {
private static final Pattern CLOSING_A_TAG_PATTERN = Pattern.compile(
"</a>", Pattern.CASE_INSENSITIVE);
private static final Pattern OPENING_A_TAG_PATTERN = Pattern.compile(
- "<a href=.*?>", Pattern.CASE_INSENSITIVE);
+ "<a\\s+href\\s*=.*?>", Pattern.CASE_INSENSITIVE | Pattern.DOTALL);
+ private static final Pattern A_HREF_PATTERN = Pattern.compile(
+ "<a\\s+href\\s*=\\s*(?:\"([^\"]*)\"|'([^']*)'|([^\\s\"'=<>`]+))\\s*>",
Review Comment:
🐞Claude Issue: **Important:** The unquoted-value alternative `[^\s"'=<>`]+` excludes `=`, so any unquoted `href` carrying a query string stops at the first `=` and the whole match fails — the link is dropped:
```
<a href=https://ex.com/?a=1>x</a> -> <a>x</a>
<a href=https://ex.com/?a=1&b=2>x</a> -> <a>x</a>
```
Both render as links on `master`, so this is a regression. Excluding `=` matches the HTML5 unquoted-attribute grammar, but browsers accept it and people write it. Dropping `=` from the class is safe here: the value still has to clear `SUBSET_LINK_PATTERN` and is re-escaped on output.
##########
app/src/main/java/org/apache/roller/weblogger/util/Utilities.java:
##########
@@ -80,7 +80,12 @@ public class Utilities {
private static final Pattern CLOSING_A_TAG_PATTERN = Pattern.compile(
"</a>", Pattern.CASE_INSENSITIVE);
private static final Pattern OPENING_A_TAG_PATTERN = Pattern.compile(
- "<a href=.*?>", Pattern.CASE_INSENSITIVE);
+ "<a\\s+href\\s*=.*?>", Pattern.CASE_INSENSITIVE | Pattern.DOTALL);
+ private static final Pattern A_HREF_PATTERN = Pattern.compile(
+ "<a\\s+href\\s*=\\s*(?:\"([^\"]*)\"|'([^']*)'|([^\\s\"'=<>`]+))\\s*>",
+ Pattern.CASE_INSENSITIVE);
Review Comment:
🐞Claude Issue: **Important:** This pattern is applied with `.matches()` (line 990), so the opening tag must be *exactly* `<a href=X>`. Any additional attribute makes the match fail and the href is discarded:
```
<a href="https://example.com/" target="_blank">x</a> -> <a>x</a>
<a href='https://example.com/' title='site'>x</a> -> <a>x</a>
<a href="https://example.com/" rel="nofollow">x</a> -> <a>x</a>
```
`title`, `target` and `rel` are common in pasted comment HTML and the link dies for all of them. Tolerating and discarding trailing attributes is safe precisely because the output anchor is reconstructed from the validated href and never echoes the input tag — adding a `(?:\s[^&]*?)?` tail before `\s*>` recovers all three cases, while `<a href="javascript:alert(1)" onclick="x">` still degrades to `<a>` via the scheme allowlist.
##########
app/src/main/java/org/apache/roller/weblogger/util/Utilities.java:
##########
@@ -975,23 +980,39 @@ public static String transformToHTMLSubset(String s) {
s = replace(s, CLOSING_LI_TAG_PATTERN, "</li>");
s = replace(s, QUOTE_PATTERN, "\"");
- // HTTP links
+ // Normalize supported links while retaining the surrounding text.
s = replace(s, CLOSING_A_TAG_PATTERN, "</a>");
Review Comment:
🐞Claude Issue: **Important:** `CLOSING_A_TAG_PATTERN` rewrites `</a>` unconditionally, but the opening tag is only rewritten when `OPENING_A_TAG_PATTERN` matches. Anchors it does not recognise therefore emit an unbalanced `</a>`:
```
<a>x</a> -> <a>x</a>
<a class="c" href="https://x/">y</a> -> <a class="c" href="https://x/">y</a>
```
This is pre-existing, but the loop is being rewritten here and already has an "unsupported anchor" output (`<a>`). Emitting that for unmatched opening tags as well would keep the markup balanced for a couple of lines of change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@roller.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Normalize links in HTML subset formatting [roller]
Posted by "snoopdave (via GitHub)" <gi...@apache.org>.
snoopdave commented on PR #190:
URL: https://github.com/apache/roller/pull/190#issuecomment-5720359737
🐞Claude Issue: **PR-Review: General Issues**
The following issues were found but cannot be attached to a specific line in the diff:
- **Blocking:** `CHANGES.md` is not updated. The repo keeps its changelog there and the `## 6.1.6` section is open, with "Behaviour changes worth reading before upgrading" and "Improvements" subsections. This PR changes how every HTML comment renders — anchors are now reconstructed from a validated href instead of passed through, and anchors that are not recognised lose their `href` — which is user-visible behaviour of exactly the kind that section documents. Please add a short entry (a sentence plus a couple of bullets), with a link to the associated GitHub issue if there is one.
- **Important:** Public API documentation does not reflect the narrowed contract. `transformToHTMLSubset` is exposed to templates as `$utils.transformToHTMLSubset` (`UtilitiesModel.java:346`) and is documented in `docs/roller-template-guide.adoc:881` as "Transform any HTML in the string to a safe HTML subset". After this change, anchor `href` values are limited to `http`, `https` and `mailto`, and every other anchor attribute is dropped. Neither the template guide nor the method javadoc (`Utilities.java:949-957`, which still describes support for `<a href>` without qualification) mentions either restriction. Both are outside the diff hunks, hence this general comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@roller.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Normalize links in HTML subset formatting [roller]
Posted by "snoopdave (via GitHub)" <gi...@apache.org>.
snoopdave merged PR #190:
URL: https://github.com/apache/roller/pull/190
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@roller.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Normalize links in HTML subset formatting [roller]
Posted by "snoopdave (via GitHub)" <gi...@apache.org>.
snoopdave commented on PR #190:
URL: https://github.com/apache/roller/pull/190#issuecomment-5720502118
🤖Claude: fixed both.
**Changelog** — added a bullet to the `## 6.1.6` "Improvements" section of `CHANGES.md` naming the three visible changes to HTML comment rendering: the scheme restriction, the dropping of non-`href` attributes, and anchors no longer leaving an unbalanced closing tag. No GitHub issue is referenced because this PR does not have an associated one.
**Docs** — updated both places:
- `docs/roller-template-guide.adoc` now states that `$utils.transformToHTMLSubset` expects already-escaped input (raw HTML passes through untouched), that anchors are rebuilt with a single `href` and no other attributes, and that only `http`, `https` and `mailto` are kept.
- The javadoc on `Utilities.transformToHTMLSubset` carries the same three points, so callers see the contract without going to the guide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@roller.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org