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

Use modern-ish jetty #377

Merged
merged 1 commit into from Oct 8, 2019
Merged

Use modern-ish jetty #377

merged 1 commit into from Oct 8, 2019

Conversation

@Ancient123
Copy link
Contributor

Ancient123 commented Oct 4, 2019

Fixes #233
Works towards #130

@Ancient123 Ancient123 requested a review from googleapis/yoshi-java as a code owner Oct 4, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 4, 2019

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no label Oct 4, 2019
@@ -4,7 +4,7 @@
<parent>
<groupId>com.google.oauth-client</groupId>
<artifactId>google-oauth-client-parent</artifactId>
<version>1.30.4-SNAPSHOT</version><!-- {x-version-update:google-oauth-client:current} -->
<version>1.30.3</version><!-- {x-version-update:google-oauth-client:current} -->

This comment has been minimized.

Copy link
@elharo

elharo Oct 5, 2019

Collaborator

I don't think this should have changed

This comment has been minimized.

Copy link
@Ancient123

Ancient123 Oct 7, 2019

Author Contributor

Fixed

<artifactId>jetty</artifactId>
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-server</artifactId>
<version>8.2.0.v20160908</version>

This comment has been minimized.

Copy link
@elharo

elharo Oct 5, 2019

Collaborator

maybe a dependencyManagement section somewhere needs to be updated?

This comment has been minimized.

Copy link
@Ancient123

Ancient123 Oct 7, 2019

Author Contributor

Found the upstream reference in the main pom.xml and fixed it there.

@@ -118,7 +118,9 @@ public String getRedirectUri() throws IOException {
server = new Server(port != -1 ? port : 0);
Connector connector = server.getConnectors()[0];
connector.setHost(host);
server.addHandler(new CallbackHandler());
//Deprecated

This comment has been minimized.

Copy link
@elharo

elharo Oct 5, 2019

Collaborator

delete the commented lines

This comment has been minimized.

Copy link
@Ancient123

Ancient123 Oct 7, 2019

Author Contributor

Fixed.

@@ -254,7 +256,7 @@ public Builder setLandingPages(String successLandingPageUrl, String failureLandi

@Override
public void handle(
String target, HttpServletRequest request, HttpServletResponse response, int dispatch)
String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response)

This comment has been minimized.

Copy link
@elharo

elharo Oct 5, 2019

Collaborator

This looks like a change in public API, which we are loath to do. Can the old method be retained and add this one, maybe one with a delegate to the other?

This comment has been minimized.

Copy link
@Ancient123

Ancient123 Oct 5, 2019

Author Contributor

I have no idea how to to that!
But, I figured after 4 years of there being a request to update a decade old highly vulnerable package ( [CVE-2011-4461][CVE-2017-7656][CVE-2017-7657][CVE-2017-7658][CVE-2017-9735][CVE-2019-10241][CVE-2019-10247] ) I might atleast post a PR to get some traction on the issue.

This comment has been minimized.

Copy link
@Ancient123

Ancient123 Oct 5, 2019

Author Contributor

Also:
This is public within class CallbackHandler which is not public. I am guessing that this is only exposed to the LocalServerReceiver class that it is nested within. Although my knowledge of java is garbage so I may be completely wrong.

This comment has been minimized.

Copy link
@elharo

elharo Oct 7, 2019

Collaborator

If the class is non-public, that's OK then.

This comment has been minimized.

Copy link
@Ancient123

Ancient123 Oct 7, 2019

Author Contributor

Looks like there is one style issue I am creating. I will try and fix it. Please re-run kokoro.

@Ancient123 Ancient123 force-pushed the Ancient123:master branch from d7d07bb to 452a502 Oct 5, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 5, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Oct 5, 2019
@Ancient123 Ancient123 force-pushed the Ancient123:master branch from 452a502 to a68feb1 Oct 5, 2019
@Ancient123 Ancient123 requested a review from elharo Oct 7, 2019
@Ancient123 Ancient123 force-pushed the Ancient123:master branch from a68feb1 to 61b2294 Oct 7, 2019
@Ancient123

This comment has been minimized.

Copy link
Contributor Author

Ancient123 commented Oct 7, 2019

Yay everything passes!

@elharo
elharo approved these changes Oct 7, 2019
Copy link
Collaborator

elharo left a comment

Long term I wonder if we should be using Jetty at all. It might be overkill for what we need. For now, though, this seems like an improvement.

@chingor13 Any final comments here?

@chingor13

This comment has been minimized.

Copy link
Collaborator

chingor13 commented Oct 8, 2019

We don't have this version of jetty internally (we do have 9.4.x) so we won't be able to import this in. We should likely update to 9.4 before we attempt to import it internally.

Copy link
Collaborator

chingor13 left a comment

I agree this is better than the alternative of leaving the old jetty.

@chingor13 chingor13 merged commit 6584664 into googleapis:master Oct 8, 2019
4 checks passed
4 checks passed
Kokoro - Test: Java 11 Build successful
Details
Kokoro - Test: Java 7 Build successful
Details
Kokoro - Test: Java 8 Build successful
Details
cla/google All necessary CLAs are signed
@Ancient123

This comment has been minimized.

Copy link
Contributor Author

Ancient123 commented Oct 8, 2019

I am still looking at migrating to 9.4.x but the 8 => 9 upgrade is pretty non-trivial.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.