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

feat: add option to pass redirect Location: header value as-is without encoding, decoding, or escaping #871

Merged
merged 3 commits into from Nov 20, 2019

Conversation

iocanel
Copy link
Contributor

@iocanel iocanel commented Oct 31, 2019

Fixes #864

At the moment I have no actual way to test it, as I failed to use 1.33.1-SNAPSHOT with Jib (any helping hand is more than welcome).

@iocanel iocanel requested a review from as a code owner Oct 31, 2019
@googlebot

This comment has been minimized.

@googlebot googlebot added the cla: no label Oct 31, 2019
elharo
elharo previously requested changes Oct 31, 2019
Copy link
Collaborator

@elharo elharo left a comment

This needs tests, and those should probably be written before the model code.

Copy link
Collaborator

@elharo elharo left a comment

I see, typo in the issue number.This is for #864, not #834

@iocanel

This comment has been minimized.

@googlebot

This comment has been minimized.

@googlebot googlebot added cla: yes and removed cla: no labels Oct 31, 2019
Copy link
Contributor

@chanseokoh chanseokoh left a comment

Thanks for doing this. I see there are still several places that need to be updated as well.

places that still need fixing (click to expand)
  public final String buildAuthority() {
    ...
    if (userInfo != null) {
      buf.append(CharEscapers.escapeUriUserInfo(userInfo)).append('@');
  private void appendRawPathFromParts(StringBuilder buf) {
      ...
      if (pathPart.length() != 0) {
        buf.append(CharEscapers.escapeUriPath(pathPart));
  public final String buildRelativeUrl() {
    ...
    if (fragment != null) {
      buf.append('#').append(URI_FRAGMENT_ESCAPER.escape(fragment));
  public static List<String> toPathParts(String encodedPath) {
      ...
      result.add(CharEscapers.decodeUri(sub));
  static void addQueryParams(Set<Entry<String, Object>> entrySet, StringBuilder buf) {
        ...
        String name = CharEscapers.escapeUriQuery(nameValueEntry.getKey());
  private static boolean appendParam(boolean first, StringBuilder buf, String name, Object value) {
    ...
    String stringValue = CharEscapers.escapeUriQuery(value.toString());

@chanseokoh

This comment has been minimized.

@iocanel

This comment has been minimized.

Copy link
Collaborator

@elharo elharo left a comment

Have you considered whether a different class or a subclass might be an option instead?

Copy link
Contributor

@chanseokoh chanseokoh left a comment

Thanks! I will start testing this on the Jib side.

Copy link
Contributor

@chanseokoh chanseokoh left a comment

Thanks again.

@chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Nov 1, 2019

I've tested the new behavior with TestWebServer.

  public static void main(String[] args) throws Exception {
    String url1 = "?id=301&_auth_=exp=1572285389~hmac=f0a387f0";
    String url2 = "?id=302&Signature=2wYOD0a%2BDAkK%2F9lQJUOuIpYti8o%3D&Expires=1569997614";
    String url3 = "?id=303&_auth_=exp=1572285389~hmac=f0a387f0";
    String url4 = "?id=307&Signature=2wYOD0a%2BDAkK%2F9lQJUOuIpYti8o%3D&Expires=1569997614";
//  String url5 = "?code=308&_auth_=exp=1572285389~hmac=f0a387f0";

    String redirect301 = "HTTP/1.1 301 Moved Permanently\nLocation: " + url1 + "\nContent-Length: 0\n\n";
    String redirect302 = "HTTP/1.1 302 Found\nLocation: " + url2 + "\nContent-Length: 0\n\n";
    String redirect303 = "HTTP/1.1 303 See Other\nLocation: " + url3 + "\nContent-Length: 0\n\n";
    String redirect307 = "HTTP/1.1 307 Temporary Redirect\nLocation: " + url4 + "\nContent-Length: 0\n\n";
//  String redirect308 = "HTTP/1.1 308 Permanent Redirect\nLocation: " + url5 + "\nContent-Length: 0\n\n";
    String ok200 = "HTTP/1.1 200 OK\nContent-Length:12\n\nHello World!";
    List<String> responses = Arrays.asList(redirect301, redirect302, redirect303, redirect307, ok200);

    try (TestWebServer server = new TestWebServer(false, responses, 1)) {
      new ApacheHttpTransport().createRequestFactory()
          .buildGetRequest(new GenericUrl(server.getEndpoint()))
          .setUseRawRedirectUrls(true).execute().disconnect();
      System.out.println(server.getInputRead());
    }
  }

The output is the following, and I can see that all the redirects preserved the raw URL values.

GET / HTTP/1.1
Accept-Encoding: gzip
User-Agent: Google-HTTP-Java-Client/1.33.1-SNAPSHOT (gzip)
Host: 0.0.0.0:41115
Connection: Keep-Alive
GET /?id=301&_auth_=exp=1572285389~hmac=f0a387f0 HTTP/1.1
Accept-Encoding: gzip
User-Agent: Google-HTTP-Java-Client/1.33.1-SNAPSHOT (gzip)
Host: 0.0.0.0:41115
Connection: Keep-Alive
GET /?id=302&Signature=2wYOD0a%2BDAkK%2F9lQJUOuIpYti8o%3D&Expires=1569997614 HTTP/1.1
Accept-Encoding: gzip
User-Agent: Google-HTTP-Java-Client/1.33.1-SNAPSHOT (gzip)
Host: 0.0.0.0:41115
Connection: Keep-Alive
GET /?id=303&_auth_=exp=1572285389~hmac=f0a387f0 HTTP/1.1
Accept-Encoding: gzip
User-Agent: Google-HTTP-Java-Client/1.33.1-SNAPSHOT (gzip)
Host: 0.0.0.0:41115
Connection: Keep-Alive
GET /?id=307&Signature=2wYOD0a%2BDAkK%2F9lQJUOuIpYti8o%3D&Expires=1569997614 HTTP/1.1
Accept-Encoding: gzip
User-Agent: Google-HTTP-Java-Client/1.33.1-SNAPSHOT (gzip)
Host: 0.0.0.0:41115
Connection: Keep-Alive

I also added a basically same test on Jib, and it also works the same way as expected. So, at least this seems to work end-to-end.

(I noticed 308 Permanent Redirect isn't automatically redirected, and this may be a bug. Filed #873.)

@iocanel however, I haven't tested Jib against actual OpenShift and Quay servers.

@iocanel
Copy link
Contributor Author

@iocanel iocanel commented Nov 4, 2019

@chanseokoh: Thanks! If you have a branch of jib working with this PR, I could possilby use it and test it against Openshift and Quay.

@chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Nov 4, 2019

@iocanel the branch is already there. Check this out: #871 (comment)

(The comment that I linked is hidden somewhere in our PR conversation history above.)

Copy link
Contributor

@chanseokoh chanseokoh left a comment

Hey @iocanel, a few things to fix.

@chanseokoh

This comment has been minimized.

@iocanel
Copy link
Contributor Author

@iocanel iocanel commented Nov 6, 2019

@iocanel I think this is working. Have you had a chance to test this against OpenShift and Quay from Jib?

Tried the fix against registry.redhat.io and it seems that it works fine.

@chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Nov 6, 2019

Thanks for verifying that. The code changes make sense to me and I think it works.

@Yoshi-Java @chingor13 please review this.

@elharo elharo added the kokoro:force-run label Nov 6, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Nov 6, 2019
@chanseokoh

This comment has been minimized.

@chanseokoh chanseokoh added the kokoro:run label Nov 11, 2019
@chanseokoh chanseokoh changed the title fix (#864): Redirect location is now passed verbatim. feat: add option to pass redirect Location: header value as-is without encoding, decoding, or escaping Nov 11, 2019
@kokoro-team kokoro-team removed the kokoro:run label Nov 11, 2019
@chanseokoh chanseokoh dismissed elharo’s stale review Nov 11, 2019

dismissing initial stale review

@chanseokoh chanseokoh added the kokoro:run label Nov 12, 2019
@kokoro-team kokoro-team removed the kokoro:run label Nov 12, 2019
@iocanel
Copy link
Contributor Author

@iocanel iocanel commented Nov 12, 2019

I think that all points have been covered.

@chanseokoh chanseokoh added the kokoro:run label Nov 12, 2019
@kokoro-team kokoro-team removed the kokoro:run label Nov 12, 2019
…nericUrl.java

Co-Authored-By: Chanseok Oh <chanseok@google.com>
@chanseokoh chanseokoh added the kokoro:run label Nov 12, 2019
@kokoro-team kokoro-team removed the kokoro:run label Nov 12, 2019
@chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Nov 14, 2019

@chingor13 @codyoss friendly ping. I'd like to get this moving.

@chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Nov 20, 2019

@chingor13 @codyoss @Yoshi-Java can we aim for having this fix for the next release? This is pretty significant stuff for us.

@codyoss codyoss merged commit 2c4f49e into googleapis:master Nov 20, 2019
10 checks passed
@codyoss
Copy link
Member

@codyoss codyoss commented Nov 20, 2019

LGMT, thanks for all of your iterations on this.

@iocanel
Copy link
Contributor Author

@iocanel iocanel commented Dec 17, 2019

@chanseokoh any idea on when this PR is going to be part of a release?

@chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Dec 17, 2019

I think this will be in the next release. But I don't know when they will make a release. @codyoss?

@codyoss
Copy link
Member

@codyoss codyoss commented Dec 17, 2019

I am going to try to get a couple more things in this release. But if they are not going to land I will make a note to try to cut a release tomorrow regardless. @chanseokoh @iocanel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants