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

Support customisation of supported protocols #4901

Open
realityfilter opened this issue Aug 3, 2022 · 22 comments
Open

Support customisation of supported protocols #4901

realityfilter opened this issue Aug 3, 2022 · 22 comments

Comments

@realityfilter
Copy link

Is your feature request related to a problem? Please describe.

Since version 0.27.0 the supported protocols are a fixed set specified in platform.

protocols: ['http', 'https', 'file', 'blob', 'url', 'data']

We are using axios in a capacitor app that uses the special capacitor protocol.

Describe the solution you'd like

We need an option to specify additional supported protocols.

Describe alternatives you've considered

Use version 0.26.2 as a workaround.

@WillianAgostini
Copy link
Contributor

Hi @realityfilter,
Adding a new parameter useProtocol in axios.create or in axios.[get,post,put...]

// Set config defaults when creating the instance
const instance = axios.create({
  baseURL: 'https://api.example.com',
  useProtocol: 'my-protocol'
});
axios.post('/user/12345', {
  name: 'new name'
}, {
  useProtocol: 'my-protocol'
})

that overrides the default protocol on the platform can resolve this issue?

If it's Ok I can create a PR.

@realityfilter
Copy link
Author

Thank you very much for the effort. For me the naming is a bit misleading.

Currently requests with 'capacitor://' protocol are leading to exceptions because of 'capacitor' is not in the fixed set of protocols for the platform. I would like to register additional valid protocols either in the instance or per request.

The name useProtocol is suggesting, that requests not specifying a protocol should use this one. That is not the intention.

const instance = axios.create({
  baseURL: 'https://api.example.com',
  registerProtocols: ['capacitor', 'custom']
});
axios.post('/user/12345', {
  name: 'new name'
}, {
    registerProtocols: ['capacitor', 'custom']
})

This is because we might have no need for a global axios instance and we target urls with different protocols.

@WillianAgostini
Copy link
Contributor

WillianAgostini commented Aug 8, 2022

Axios uses url lib to parse an URL, so I can pass protocol by default from the url base, here is an example. Look at tag protocol.

> url.parse('https://stackoverflow.com/questions/36588396/node-js-url-parsing');
Url {
  protocol: 'https:',
  slashes: true,
  auth: null,
  host: 'stackoverflow.com',
  port: null,
  hostname: 'stackoverflow.com',
  hash: null,
  search: null,
  query: null,
  pathname: '/questions/36588396/node-js-url-parsing',
  path: '/questions/36588396/node-js-url-parsing',
  href: 'https://stackoverflow.com/questions/36588396/node-js-url-parsing'
}
> const url = require('url');
> url.parse('capacitor://stackoverflow.com/');
Url {
  protocol: 'capacitor:',
  slashes: true,
  auth: null,
  host: 'stackoverflow.com',
  port: null,
  hostname: 'stackoverflow.com',
  hash: null,
  search: null,
  query: null,
  pathname: '/',
  path: '/',
  href: 'capacitor://stackoverflow.com/'
}

It resolves this issue or should we proceed to add a new feature?

@realityfilter
Copy link
Author

This is the problematic section:

axios/lib/adapters/xhr.js

Lines 240 to 245 in cd8989a

const protocol = parseProtocol(fullPath);
if (protocol && platform.protocols.indexOf(protocol) === -1) {
reject(new AxiosError('Unsupported protocol ' + protocol + ':', AxiosError.ERR_BAD_REQUEST, config));
return;
}

The changelog states:

Version 0.27.1:

So I think it is better to add a new feature?

@WillianAgostini
Copy link
Contributor

WillianAgostini commented Aug 10, 2022

It's a better idea to add a new feature, adding registerProtocols in platform.protocols.
Will be provided 3 ways to configure registerProtocols:

Creating an instance

const instance = axios.create({
  baseURL: 'https://api.example.com',
  registerProtocols: ['capacitor', 'custom']
});

Instance methods

axios.post('/user/12345', {
  name: 'new name'
}, {
    registerProtocols: ['capacitor', 'custom']
})

Global axios defaults

axios.defaults.registerProtocols: ['capacitor', 'custom']

I'll make the changes to this feature.

@realityfilter
Copy link
Author

That is exactly what is needed. Thank you very much.

@WillianAgostini
Copy link
Contributor

@realityfilter, There's some example that I can use to test with the client and server that can reproduce the issue #4901 (comment)?

@realityfilter
Copy link
Author

realityfilter commented Aug 11, 2022

describe('axios', () => {
    it('should know protocol capacitor', async () => {
        try {
            await axios.get('capacitor://localhost/test.js')
        } catch (e) {
            console.log(e)
            expect(e.code).not.toBe('ERR_BAD_REQUEST')
            expect(e.message).not.toBe('Unsupported protocol capacitor:')
        }
    })
})

@WillianAgostini
Copy link
Contributor

Has a limitation on one of the libraries used by axios.

On line https://github.com/WillianAgostini/axios/blob/871ef051124fa9f02dcc05c67f29ff9ce3e681b5/lib/adapters/http.js#L292 has a call to lib follow-redirects, but she has a limitation to use only https or http connection https://github.com/follow-redirects/follow-redirects/blob/main/index.js#L597.

@realityfilter
Copy link
Author

That would be no problem in this use case.

@honungsburk
Copy link

I have a use case that requires handling custom protocols and redirect. The SIOPv2 spec uses redirects with openid://... for same-device communication (this allows smartphones to open the correct app). In my case, I'm writing a simple CLI tool and need to deal with the redirect myself.

Right now I solved it by simply setting maxRedirects to 0 and then catching the error

Simple example:

  let reqURI = "";
  await axios
    .post(path, undefined, {
      maxRedirects: 0,
    })
    .catch((err) => {
      if (err.response.status === 302) {
        // We should have been redirected to "openid://?response_type=id_token..."
        reqURI = err.response.headers.location;
      } else {
        throw err;
      }
    });

I don't need any features to be added (I think at least) but I thought you guys should know the use case exists.

@dhondtlaurens
Copy link

dhondtlaurens commented Sep 8, 2022

@WillianAgostini, Is there a PR already? Or can we help with something to add this feature? I'm having the same issue with a custom protocol on android devices and I found the rules @realityfilter posted to be responsible:

axios/lib/adapters/xhr.js

Lines 240 to 245 in cd8989a

const protocol = parseProtocol(fullPath);
if (protocol && platform.protocols.indexOf(protocol) === -1) {
reject(new AxiosError('Unsupported protocol ' + protocol + ':', AxiosError.ERR_BAD_REQUEST, config));
return;
}

Thanks in advance!

@WillianAgostini
Copy link
Contributor

WillianAgostini commented Sep 10, 2022

Hi @dhondtlaurens
I committed WillianAgostini@2d7277e to customize protocols on xhr adapter.
But it is still missing a test case for this feature, with a server that supports the "capacitor" protocol to respond.

@dhondtlaurens
Copy link

Any reason why the test case of @realityfilter is not enough? Because this is not a capacitor-specific problem. All requests with a different URL scheme (protocol) are impacted, so the test should only check the following things:

    1. When registerProtocols is not set, expect the error to be ERR_BAD_REQUEST + Unsupported protocol xxx
    1. When registerProtocols is set, expect the error not to be ERR_BAD REQUEST + Unsupported protocol xxx

Any thoughts on this?

@exander77
Copy link

Does this work? I have a custom protocol after redirect.

I also think maxRedidirects: n is broken. In my opinion it should do at most n redirects, not issue error, when there are more redirects available.

@exander77
Copy link

I am looking into beforeRedirect, but is not clear if I can cancel redirect there?

@sebastienlabine
Copy link

sebastienlabine commented Feb 24, 2023

I truly don't think register protocol is the way. We should be able to handle any valid protocol, like @honungsburk 's use case.

@exander77
Copy link

I truly don't think register protocol is the way. We should be able to handle any valid protocol, like @honungsburk 's use case.

Issue is that any protocol is valid.

@alevys
Copy link

alevys commented Jul 11, 2023

Hi! Are there any movements on this issue? It is very necessary to use a custom protocol, like - vkcors://, but not with new versions. You have to use only the old version 0.26.1, which still has support for custom protocols. Please add the ability to use custom protocols. Thank you.

@Space1nvader
Copy link

Hi! Are there any movements on this issue? It is very necessary to use a custom protocol, like - vkcors://, but not with new versions. You have to use only the old version 0.26.1, which still has support for custom protocols. Please add the ability to use custom protocols. Thank you.

Thank you very match for your solution

@alevys
Copy link

alevys commented Oct 19, 2023

Hello! Are there any changes planned on this issue? Thank you.

@andrewgul
Copy link

@alevys Hello! Maybe you found some alternative solution?

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

No branches or pull requests

9 participants