Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed fix for WW-5022 (escape html tag body control flag) #373

Merged

Conversation

JCgH4164838Gh792C124B5
Copy link
Contributor

Proposed fix for WW-5022 (escape html tag body control flag)

  • Added escapeHtmlBody parameter to s:a and s:submit tags.
  • No other tags appear to require this feature (but can be added to any component).
  • Added new unit tests for escapeHtmlBody (and usesBody for component).
  • Fixed broken s:a tags in ShowCase app.

- Added escapeHtmlBody parameter to s:a and s:submit tags.
- No other tags appear to require this feature (but can be added to any
component).
- Added new unit tests for escapeHtmlBody (and usesBody for component).
- Fixed broken s:a tags in ShowCase app.
@JCgH4164838Gh792C124B5
Copy link
Contributor Author

Hello Apache Struts Team.

This PR is a starting point for a fix to WW-5022 (automatic html escaping of some tag bodies breaks content display in 2.6.x).

Previous commentary in the JIRA by L. Lenart and Y. Zamani identified s:a and s:submit tags as being impacted and I could not find any others during this PR's construction. If anyone can spot other tags that may need the new parameter please advise with a comment.

The FreeMarker template changes seem to be OK, but please feel free to suggest a more efficient alternative.

Thanks.

@lukaszlenart
Copy link
Member

Nice work! My only concern is maybe we should set the defaults to false to keep backward compatibility, wdyt?

@JCgH4164838Gh792C124B5
Copy link
Contributor Author

Hello @lukaszlenart.

When reading the comments in the JIRA it seemed that both you and Yasser were more in favour of HTML-escaping by default, with an "opt-out" for the tags that might need it. The PR was configured based on that interpretation.

The defaults could be set to false, but maybe auto-escaping by default (defaults set to true as in the current PR state) is a little safer for content-handling in 2.6 ?

If 2.6.0 has the defaults set to true but developer feedback requests it be changed it could be flipped false in a later 2.6.x release (without impacting anyone who explicitly set a desired override value in their tags). If there's a 2.5.x to 2.6.x migration guide this change could be highlighted there.

The default can be left as-is (true) or changed (false) can be set either way to start with in the PR, just let me know what you prefer.

@lukaszlenart
Copy link
Member

@JCgH4164838Gh792C124B5 thanks a lot for this in deep explanation, I think you've pointed it out and I agree that leaving true as a default value makes perfect sense :)

@lukaszlenart
Copy link
Member

LGTM 👍

@lukaszlenart lukaszlenart merged commit 2d23357 into apache:master Oct 19, 2019
@JCgH4164838Gh792C124B5 JCgH4164838Gh792C124B5 deleted the local_26x_TagBodyEscapeCtrl branch October 26, 2019 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants