[JENKINS-65790]: Remove injected styles and scripts in form validation

[sabberworm]

See JENKINS-65790.

In case of errors during form validation, the response was used verbatim. Improves this behaviour somewhat by stripping styles and scripts from the response. Also uses a <details> element instead of a click handler on an <a> tag to show/hide the error response.

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

• [x] (If applicable) Jira issue is well described
• [x] Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
  • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
• [ ] Appropriate autotests or explanation to why this change has no tests
• [ ] For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@daniel-beck

Maintainer checklist

Before the changes are marked as ready-for-merge:

• [ ] There are at least 2 approvals for the pull request and no outstanding requests for change
• [ ] Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
• [ ] Changelog entries in the PR title and/or Proposed changelog entries are correct
• [ ] Proper changelog labels are set so that the changelog can be generated automatically
• [ ] If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
• [ ] If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[alecharp]

@daniel-beck @Wadeck do you have any concerns about the content of this PR?

[basil]

Needs a review from @jenkinsci-cert.

[timja]

The linked Jira doesn't seem to be related to this.

Also, form validation had some re-work done recently so this is now conflicted

[sabberworm]

The linked Jira doesn't seem to be related to this.

Why not? The linked Jira is caused by a global style from the default sinatra error page being injected into the page. The change I propose would ensure no styles get injected.

It doesn’t fix the whole issue mentioned in the Jira, since you could argue it is also caused by some old plugins’ form validation server code expecting a GET request where the client now sends a POST (but that part was deemed wontfix)…

[sabberworm]

Also, form validation had some re-work done recently so this is now conflicted

To be honest, I’m not very motivated to rework my change right now. My PR has been open for almost a year and could have been merged at any time. It wasn’t conflicting till 4 days ago (848883db8d748316175930aa5fef3749ec5cdc46 by @janfaracik – who was pinged on this PR 2 months ago, long before their change).

Who’s to say that, after me fixing the conflict right now, the PR won’t remain open for another year until someone does the next conflicting change, at which point I will be asked to rework again?

[NotMyFault]

While we have the go from the security team for the proposed change, a more contemporary diff has been implemented in the meantime to validate form input. If the jira issue applies to the new form validation still, the PR needs to be revised to adapt the recent changes, because the current proposal is no longer applicable.

[Wadeck]

(side topic) @NotMyFault It could be interesting to find a way to prevent more recent initiatives to make previous PRs irrelevant, instead of re-using / adapting them to suit the new goal. That could reduce frustration from non-very-active contributors who's effort should also be valued. Any idea could help ;) Topic added for the next UI/UX SIG meeting

[NotMyFault]

That could reduce frustration from non-very-active contributors who's effort should also be valued. Any idea could help ;)

Yeah, that's an understandable and actually relatable concern. Our backlog of PRs decreased a lot (thanks to everyone triaging, reviewing and integrating PRs 👏🏻), but basil's review request of a security team member basically brought this PR back to life.

To be fair, keeping such a small PR (diff wise) on hold for almost a year without feedback the submitter can address doesn't facilitate a fast review process or a smooth integration without conflicts at all.

[basil]

Who’s to say that, after me fixing the conflict right now, the PR won’t remain open for another year until someone does the next conflicting change, at which point I will be asked to rework again?

You are right, and on behalf of the core team I apologize for this. We will try to do better in the future. I have been making a concerted effort to shepherd recent PRs across the finish line in a timely fashion and to address the PR backlog. I am grateful to my colleagues on the Jenkins core and security teams for their valued coöperation in this regard.

If the jira issue applies to the new form validation still, the PR needs to be revised to adapt the recent changes, because the current proposal is no longer applicable.

So does the Jira issue apply to the new form validation, or does it not? After reading this thread carefully the answer to this question remains unclear to me. Based on JENKINS-65790 (comment) it seems that the issue described in JENKINS-65790 only applies when deprecated plugins are installed, and I have not seen any indication that the issue can be reproduced on an installation without any deprecated plugins. If the issue can still be reproduced on the latest weekly without any deprecated plugins installed, then the issue remains valid and we can discuss the next steps for the implementation in this PR. If the issue cannot be reproduced on the latest weekly without any deprecated plugins installed, then in approximately a week I plan to close both the issue (as "Cannot Reproduce") and this PR.

[timja]

Closing as there's no diff remaining in this PR and no update in awhile. Apologies for the PR being missed earlier