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

Redirect 302 changes method from POST to GET #138

Closed
dankle opened this issue Jul 2, 2016 · 8 comments
Closed

Redirect 302 changes method from POST to GET #138

dankle opened this issue Jul 2, 2016 · 8 comments
Assignees
Labels
bug
Milestone

Comments

@dankle
Copy link

@dankle dankle commented Jul 2, 2016

I'm having some problems when following https redirects with wreck.

I have this code:

server.ext('onRequest', function (request, reply) {
    Wreck.read(request.raw.req, null, function (err, reqBody) {

      var requestOptions = {
        baseUrl: request.host,
        headers: request.headers,
        redirects: 10,
        beforeRedirect: function (redirectMethod, statusCode, location, resHeaders, redirectOptions, next) {
          console.log('location: ' + location);
          return next();
        },
        timeout: 100000,
        secureProtocol: 'SSLv3_method'
      };

      if(request.method === 'post') {
        requestOptions.payload = reqBody;
      }

      var optionalCallback = function (err, res) {
        return reply(res);
      };

      return Wreck.request(request.method, request.url.href, requestOptions, optionalCallback);

    });

It all works fine until I get a 302 Moved Temporarily which points the request to https://www.testsite.com/test.The location log above in beforeRedirectalso prints https://www.testsite.com/test but the actual url for the Wreck redirect will be http://www.testsite.com:443/test (note that it is still using http). This will end up in socket hangup.

Am I missing something in my the code or is this a bug in Wreck?

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Jul 2, 2016

I cannot reproduce the issue locally with the following code. Have you tried disabling the secureProtocol option, you shouldn't need to set that.

'use strict';

const Http = require('http');
const Https = require('https');
const Wreck = require('wreck');


const httpServer = Http.createServer((req, res) => {
  res.writeHead(302, { Location: 'https://localhost/test' });
  res.end();
});
httpServer.listen(80, () => {
  const httpsOptions = {
    key: '-----BEGIN RSA PRIVATE KEY-----\nMIIEpAIBAAKCAQEA0UqyXDCqWDKpoNQQK/fdr0OkG4gW6DUafxdufH9GmkX/zoKz\ng/SFLrPipzSGINKWtyMvo7mPjXqqVgE10LDI3VFV8IR6fnART+AF8CW5HMBPGt/s\nfQW4W4puvBHkBxWSW1EvbecgNEIS9hTGvHXkFzm4xJ2e9DHp2xoVAjREC73B7JbF\nhc5ZGGchKw+CFmAiNysU0DmBgQcac0eg2pWoT+YGmTeQj6sRXO67n2xy/hA1DuN6\nA4WBK3wM3O4BnTG0dNbWUEbe7yAbV5gEyq57GhJIeYxRvveVDaX90LoAqM4cUH06\n6rciON0UbDHV2LP/JaH5jzBjUyCnKLLo5snlbwIDAQABAoIBAQDJm7YC3pJJUcxb\nc8x8PlHbUkJUjxzZ5MW4Zb71yLkfRYzsxrTcyQA+g+QzA4KtPY8XrZpnkgm51M8e\n+B16AcIMiBxMC6HgCF503i16LyyJiKrrDYfGy2rTK6AOJQHO3TXWJ3eT3BAGpxuS\n12K2Cq6EvQLCy79iJm7Ks+5G6EggMZPfCVdEhffRm2Epl4T7LpIAqWiUDcDfS05n\nNNfAGxxvALPn+D+kzcSF6hpmCVrFVTf9ouhvnr+0DpIIVPwSK/REAF3Ux5SQvFuL\njPmh3bGwfRtcC5d21QNrHdoBVSN2UBLmbHUpBUcOBI8FyivAWJhRfKnhTvXMFG8L\nwaXB51IZAoGBAP/E3uz6zCyN7l2j09wmbyNOi1AKvr1WSmuBJveITouwblnRSdvc\nsYm4YYE0Vb94AG4n7JIfZLKtTN0xvnCo8tYjrdwMJyGfEfMGCQQ9MpOBXAkVVZvP\ne2k4zHNNsfvSc38UNSt7K0HkVuH5BkRBQeskcsyMeu0qK4wQwdtiCoBDAoGBANF7\nFMppYxSW4ir7Jvkh0P8bP/Z7AtaSmkX7iMmUYT+gMFB5EKqFTQjNQgSJxS/uHVDE\nSC5co8WGHnRk7YH2Pp+Ty1fHfXNWyoOOzNEWvg6CFeMHW2o+/qZd4Z5Fep6qCLaa\nFvzWWC2S5YslEaaP8DQ74aAX4o+/TECrxi0z2lllAoGAdRB6qCSyRsI/k4Rkd6Lv\nw00z3lLMsoRIU6QtXaZ5rN335Awyrfr5F3vYxPZbOOOH7uM/GDJeOJmxUJxv+cia\nPQDflpPJZU4VPRJKFjKcb38JzO6C3Gm+po5kpXGuQQA19LgfDeO2DNaiHZOJFrx3\nm1R3Zr/1k491lwokcHETNVkCgYBPLjrZl6Q/8BhlLrG4kbOx+dbfj/euq5NsyHsX\n1uI7bo1Una5TBjfsD8nYdUr3pwWltcui2pl83Ak+7bdo3G8nWnIOJ/WfVzsNJzj7\n/6CvUzR6sBk5u739nJbfgFutBZBtlSkDQPHrqA7j3Ysibl3ZIJlULjMRKrnj6Ans\npCDwkQKBgQCM7gu3p7veYwCZaxqDMz5/GGFUB1My7sK0hcT7/oH61yw3O8pOekee\nuctI1R3NOudn1cs5TAy/aypgLDYTUGQTiBRILeMiZnOrvQQB9cEf7TFgDoRNCcDs\nV/ZWiegVB/WY7H0BkCekuq5bHwjgtJTpvHGqQ9YD7RhE8RSYOhdQ/Q==\n-----END RSA PRIVATE KEY-----\n',
    cert: '-----BEGIN CERTIFICATE-----\nMIIDBjCCAe4CCQDvLNml6smHlTANBgkqhkiG9w0BAQUFADBFMQswCQYDVQQGEwJV\nUzETMBEGA1UECAwKU29tZS1TdGF0ZTEhMB8GA1UECgwYSW50ZXJuZXQgV2lkZ2l0\ncyBQdHkgTHRkMB4XDTE0MDEyNTIxMjIxOFoXDTE1MDEyNTIxMjIxOFowRTELMAkG\nA1UEBhMCVVMxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoMGEludGVybmV0\nIFdpZGdpdHMgUHR5IEx0ZDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEB\nANFKslwwqlgyqaDUECv33a9DpBuIFug1Gn8Xbnx/RppF/86Cs4P0hS6z4qc0hiDS\nlrcjL6O5j416qlYBNdCwyN1RVfCEen5wEU/gBfAluRzATxrf7H0FuFuKbrwR5AcV\nkltRL23nIDRCEvYUxrx15Bc5uMSdnvQx6dsaFQI0RAu9weyWxYXOWRhnISsPghZg\nIjcrFNA5gYEHGnNHoNqVqE/mBpk3kI+rEVzuu59scv4QNQ7jegOFgSt8DNzuAZ0x\ntHTW1lBG3u8gG1eYBMquexoSSHmMUb73lQ2l/dC6AKjOHFB9Ouq3IjjdFGwx1diz\n/yWh+Y8wY1Mgpyiy6ObJ5W8CAwEAATANBgkqhkiG9w0BAQUFAAOCAQEAoSc6Skb4\ng1e0ZqPKXBV2qbx7hlqIyYpubCl1rDiEdVzqYYZEwmst36fJRRrVaFuAM/1DYAmT\nWMhU+yTfA+vCS4tql9b9zUhPw/IDHpBDWyR01spoZFBF/hE1MGNpCSXXsAbmCiVf\naxrIgR2DNketbDxkQx671KwF1+1JOMo9ffXp+OhuRo5NaGIxhTsZ+f/MA4y084Aj\nDI39av50sTRTWWShlN+J7PtdQVA5SZD97oYbeUeL7gI18kAJww9eUdmT0nEjcwKs\nxsQT1fyKbo7AlZBY4KSlUMuGnn0VnAsB9b+LxtXlDfnjyM8bVQx1uAfRo0DO8p/5\n3J5DTjAU55deBQ==\n-----END CERTIFICATE-----\n'
  };
  const httpsServer = Https.createServer(httpsOptions, (req, res) => {
    res.writeHead(200);
    res.end('Success!');
  });

  httpsServer.listen(443, () => {
    const options = {
      beforeRedirect: function (redirectMethod, statusCode, location, resHeaders, redirectOptions, next) {
        console.log('location: ' + location);
        return next();
      },
      redirects: 10,
      rejectUnauthorized: false
    };

    Wreck.get('http://localhost/test', options, (err, res, body) => {
      console.log(body.toString());
    });
  });
});

You will need to run the above test with sudo since it binds to port 80 and 443

@dankle

This comment has been minimized.

Copy link
Author

@dankle dankle commented Jul 3, 2016

Thanks @geek for the example, you saved my day. I had to remove secureProtocol: 'SSLv3_methodand baseUrl: request.host. I would think that the baseUrlwas creating the strange behavior.

The following code is working for the url, but I found another issue. When a POST request is redirected the redirectMethodwill change to GET:

server.ext('onRequest', function (request, reply) {
    Wreck.read(request.raw.req, null, function (err, reqBody) {

      var requestOptions = {
        headers: request.headers,
        redirects: 10,
        beforeRedirect: function (redirectMethod, statusCode, location, resHeaders, redirectOptions, next) {
          console.log('location: ' + location);
          return next();
        }
      };

      if(request.method === 'post') {
        requestOptions.payload = reqBody;
      }

      var optionalCallback = function (err, res) {
        return reply(res);
      };

      return Wreck.request(request.method, request.url.href, requestOptions, optionalCallback);

    });
});

To get it to work I had to change this line:
https://github.com/hapijs/wreck/blob/master/lib/index.js#L286

To:

switch (code) {
        case 301:
        case 302:
            return 'method';

When I read the HTTP specs http://tools.ietf.org/html/rfc2616#page-62, it says that most user agents behave like this. But it would be good for me as a user to perform a POSTon a 302 redirect if our backend by mistake are returning 302 instead of 307or 308.

I noticed that it's not possible to change the redirectMethod from within beforeRedirect'. The method I assign manually toredirectMethod` will be overwritten by https://github.com/hapijs/wreck/blob/master/lib/index.js#L286.

Can we maybe do a change here to make this functionality a little bit more flexible?

@dankle dankle changed the title Problem with redirects from http -> https Redirect 302 changes method from POST to GET Jul 7, 2016
@nlf

This comment has been minimized.

Copy link
Member

@nlf nlf commented Mar 21, 2017

the RFC does, in fact, state that the method should not be changed. sounds like this is technically a bug.

@nlf nlf added the bug label Mar 21, 2017
@geek geek self-assigned this Mar 21, 2017
@geek geek added this to the 11.0.0 milestone Mar 21, 2017
@geek geek closed this in #161 Mar 21, 2017
@kanongil

This comment has been minimized.

Copy link
Member

@kanongil kanongil commented Mar 21, 2017

Please don't use obsoleted RFC's for reference.

https://tools.ietf.org/html/rfc7231#section-6.4.2 states that changing the method to GET is fine.

@nlf

This comment has been minimized.

Copy link
Member

@nlf nlf commented Mar 21, 2017

IMO "For historical reasons, a user agent MAY change the request method from POST to GET for the subsequent request." doesn't make it the correct behavior, i'm still a +1 for keeping the original method

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Mar 21, 2017

@kanongil I went back and forth on the change, and considered adding an extension to change the method... but the fact remains that we disable redirects by default and therefore the user must opt-in to performing a redirect, indicating that they trust the server.

@kanongil

This comment has been minimized.

Copy link
Member

@kanongil kanongil commented Mar 21, 2017

Yes, I don't mind the change, just the reference :-)

The UA (wreck) is allowed to handle it either way.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Mar 21, 2017

@kanongil thanks, I updated the release notes with the better reference #162

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