Use modern-ish jetty #377

Ancient123 · 2019-10-04T22:03:00Z

Fixes #233
Works towards #130

googlebot · 2019-10-04T22:03:06Z

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

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

elharo · 2019-10-05T00:26:37Z

google-oauth-client-jetty/pom.xml

@@ -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} -->


I don't think this should have changed

elharo · 2019-10-05T00:27:03Z

google-oauth-client-jetty/pom.xml

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


maybe a dependencyManagement section somewhere needs to be updated?

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

elharo · 2019-10-05T00:27:18Z

...ty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java

@@ -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


delete the commented lines

elharo · 2019-10-05T00:28:15Z

...ty/src/main/java/com/google/api/client/extensions/jetty/auth/oauth2/LocalServerReceiver.java

@@ -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 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?

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.

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.

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

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

googlebot · 2019-10-05T00:40:31Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Ancient123 · 2019-10-07T19:57:06Z

Yay everything passes!

elharo

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 · 2019-10-08T00:09:16Z

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.

chingor13

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

Ancient123 · 2019-10-08T00:20:48Z

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

Ancient123 requested a review from a team as a code owner October 4, 2019 22:03

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Oct 4, 2019

elharo reviewed Oct 5, 2019

View reviewed changes

elharo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 5, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 5, 2019

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Oct 5, 2019

Ancient123 requested a review from elharo October 7, 2019 16:47

elharo added kokoro:force-run Add this label to force Kokoro to re-run the tests. semver: patch A minor bug fix or small change. labels Oct 7, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 7, 2019

Use modern-ish jetty

61b2294

elharo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 7, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 7, 2019

elharo approved these changes Oct 7, 2019

View reviewed changes

chingor13 approved these changes Oct 8, 2019

View reviewed changes

chingor13 merged commit 6584664 into googleapis:master Oct 8, 2019

release-please bot mentioned this pull request Jan 4, 2022

chore(main): release google-oauth-java-client 1.33.0 #791

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use modern-ish jetty #377

Use modern-ish jetty #377

Ancient123 commented Oct 4, 2019

googlebot commented Oct 4, 2019

elharo Oct 5, 2019

Ancient123 Oct 7, 2019

elharo Oct 5, 2019

Ancient123 Oct 7, 2019

elharo Oct 5, 2019

Ancient123 Oct 7, 2019

elharo Oct 5, 2019

Ancient123 Oct 5, 2019

Ancient123 Oct 5, 2019

elharo Oct 7, 2019

Ancient123 Oct 7, 2019

googlebot commented Oct 5, 2019

Ancient123 commented Oct 7, 2019

elharo left a comment

chingor13 commented Oct 8, 2019

chingor13 left a comment

Ancient123 commented Oct 8, 2019

Use modern-ish jetty #377

Use modern-ish jetty #377

Conversation

Ancient123 commented Oct 4, 2019

googlebot commented Oct 4, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Oct 5, 2019

Ancient123 commented Oct 7, 2019

elharo left a comment

Choose a reason for hiding this comment

chingor13 commented Oct 8, 2019

chingor13 left a comment

Choose a reason for hiding this comment

Ancient123 commented Oct 8, 2019