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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-60866] Make st:bind tag and JavaScript proxy work without inline JS #385

Merged
merged 31 commits into from
Feb 14, 2024
Merged

[JENKINS-60866] Make st:bind tag and JavaScript proxy work without inline JS #385

merged 31 commits into from
Feb 14, 2024

Conversation

daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Jul 16, 2022 •
edited
Loading

See JENKINS-60866.

Currently, st:bind generates inline JS. This doesn't work with CSP, so move that out.

Additionally, add a new StaplerRequest#createJavaScriptProxyParameters as replacement for StaplerRequest#createJavaScriptProxy that can be used without needing to eval the result.

Downstream PR: jenkinsci/jenkins#6865

@daniel-beck daniel-beck requested a review from Wadeck July 16, 2022 21:28
@daniel-beck daniel-beck mentioned this pull request Jul 16, 2022
Merged
12 tasks
@daniel-beck daniel-beck changed the title [JENKINS-60866] Make st:bind tag work without inline JS [JENKINS-60866] Make st:bind tag and JavaScript proxy work without inline JS Jul 17, 2022
jglick
jglick reviewed Jul 25, 2022
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Do not recall well enough how st:bind works to offer a meaningful review

core/src/main/java/org/kohsuke/stapler/StaplerRequest.java Outdated Show resolved Hide resolved
@daniel-beck

This comment was marked as resolved.

Sign in to view
@Wadeck

This comment was marked as resolved.

Sign in to view
@timja

This comment was marked as resolved.

Sign in to view
@daniel-beck

This comment was marked as resolved.

Sign in to view
@timja timja mentioned this pull request Apr 25, 2023
Closed
@daniel-beck

This comment was marked as resolved.

Sign in to view
@daniel-beck daniel-beck marked this pull request as ready for review August 14, 2023 10:00
jglick
jglick reviewed Aug 14, 2023
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Not sure I follow well enough to review seriously.

core/src/main/java/org/kohsuke/stapler/bind/Bound.java Outdated Show resolved Hide resolved
jelly/src/main/java/org/kohsuke/stapler/jelly/BindTag.java Outdated Show resolved Hide resolved
@daniel-beck

This comment was marked as resolved.

Sign in to view
@daniel-beck daniel-beck marked this pull request as draft August 14, 2023 20:51
@daniel-beck daniel-beck marked this pull request as ready for review August 21, 2023 12:22
yaroslavafenkin
yaroslavafenkin approved these changes Aug 22, 2023
View reviewed changes
Copy link

@yaroslavafenkin yaroslavafenkin left a comment

Choose a reason for hiding this comment

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

Tested locally, seems to work as expected for cases I had in mind.

core/src/main/java/org/kohsuke/stapler/bind/BoundObjectTable.java Outdated Show resolved Hide resolved
@daniel-beck daniel-beck marked this pull request as draft August 22, 2023 17:21
@daniel-beck daniel-beck marked this pull request as ready for review August 25, 2023 09:32
timja
timja approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@timja timja left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

This was referenced Sep 26, 2023
Closed
Merged
jglick
jglick reviewed Nov 1, 2023
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Presuming someone else has done code review & testing on this & downstream, do you need a merge & release? It is not clear to me whether this is still a WiP or ready to go.

core/src/main/java/org/kohsuke/stapler/bind/Bound.java Outdated Show resolved Hide resolved
core/src/main/java/org/kohsuke/stapler/bind/Bound.java Outdated Show resolved Hide resolved
@daniel-beck
Copy link
Member Author

do you need a merge & release? It is not clear to me whether this is still a WiP or ready to go.

Thanks, just no good time IMO in the recent weeks. I'd like to be around for post-release support and ideally not simultaneously with another Stapler release.

@daniel-beck daniel-beck requested a review from timja February 13, 2024 08:35
@daniel-beck
Copy link
Member Author

Last call for reviews :)

jglick
jglick reviewed Feb 13, 2024
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Superficial comments, since I do not remember much about this system.

core/src/main/java/org/kohsuke/stapler/bind/Bound.java Outdated Show resolved Hide resolved
core/src/main/java/org/kohsuke/stapler/bind/Bound.java Outdated Show resolved Hide resolved
core/src/main/java/org/kohsuke/stapler/bind/Bound.java Show resolved Hide resolved
jelly/src/main/java/org/kohsuke/stapler/jelly/BindTag.java Outdated Show resolved Hide resolved
@daniel-beck daniel-beck merged commit ed17667 into jenkinsci:master Feb 14, 2024
13 checks passed
@MarkEWaite MarkEWaite linked an issue Feb 14, 2024 that may be closed by this pull request
stapler:bind is creating inline javascript #457
Closed
@shlomomdahan shlomomdahan mentioned this pull request Nov 5, 2024
Draft
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jglick jglick jglick left review comments

@yaroslavafenkin yaroslavafenkin yaroslavafenkin approved these changes

@timja timja timja approved these changes

@Wadeck Wadeck Awaiting requested review from Wadeck

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

stapler:bind is creating inline javascript
6 participants
@daniel-beck @Wadeck @timja @jglick @yaroslavafenkin @basil