Skip to content
This repository has been archived by the owner on Mar 12, 2021. It is now read-only.

Make User-Agent configurable #171

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

MathyV
Copy link
Contributor

@MathyV MathyV commented Nov 14, 2017

No description provided.

@moozzyk
Copy link
Contributor

moozzyk commented Nov 14, 2017

Why do you need it?

@MathyV
Copy link
Contributor Author

MathyV commented Nov 14, 2017

Well, I can think of 2 good reasons:

  1. Some hosts require that the User Agent is set to a specific value. In my case the host is behind a Cloudflare instance which you have to get a cookie for in your regular browser, which you then copy into the headers to be able to run an external program. The User Agent however also has to match for it to work.
  2. You want to properly identify your program when accessing a remote API

I (kind of) compare it to the functionality that tools like wget and curl provide.

@moozzyk
Copy link
Contributor

moozzyk commented Nov 15, 2017

Looking from the browser perspective - browsers typically do not allow setting the User-Agent header when sending HTTP requests with XmlHttpRequest.

  1. This seems like hijacking a different session. Where are you taking the header and cookie values from? Instead of doing this you could send the initial http request with the same header SignalR is using and then copy headers and cookies from the response.
  2. If I understand this correctly - on the server side you want to identify the application/browser that sent that request. If this is the case it seems that this change actually prevents achieving this goal since anyone can spoof the User-Agent and you can't be sure what application sent the request. (One of the reasons why the header is currently hard coded and contains a version is to be able to tell immediately what client (C# vs. C++) and client version the user is using when investigating an issue)

@MathyV
Copy link
Contributor Author

MathyV commented Nov 15, 2017

  1. I'm taking the cookie from my own browser and using them in the command-line tool I've developed. Cloudflare requires a JavaScript engine to get the cookies, which I don't have, hence I manually copy/paste the values.
  2. I would want to identify my application, not which version of a library I use in it.

Some extra food for thought:

  • cpprestsdk allows setting the user agent, I just made that functionality available through this library
  • I noticed yesterday that the websocket part of the communication actually uses a different user agent (the default in websocketpp) because it is not initialized. I created another PR in cpprestsdk to add functionality that allows setting it. Make it possible to set the user agent for websockets microsoft/cpprestsdk#606

@moozzyk
Copy link
Contributor

moozzyk commented Nov 17, 2017

@MathyV
I think identifying an application is a moot point - you could use other means (a custom header, parameter etc.) for this
cpprestsdk allows setting headers since they are library that enables sending http requests and as such they (have to) make it possible to prepare the request in any way the consumer see it fit.

I am still on a fence here but before this can be merged you need to sign the cla

@MathyV
Copy link
Contributor Author

MathyV commented Nov 17, 2017

I already signed the cla, dunno why it is not showing as such.

@moozzyk
Copy link
Contributor

moozzyk commented Nov 17, 2017

OK - closing and reopening the PR fixed the CLA label

@moozzyk
Copy link
Contributor

moozzyk commented Nov 17, 2017

I guess there is no harm in taking it especially that the setting is only exposed on the config...

Copy link
Contributor

@moozzyk moozzyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure that tests compile and pass.

@@ -26,7 +25,6 @@ namespace signalr
private:
const web::uri m_url;
web::http::http_request m_request;
utility::string_t m_user_agent_string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to fix this test since you removed this method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check it, overlooked the tests

@ghost ghost deleted a comment from dnfclas Dec 5, 2017
@ghost ghost deleted a comment from dnfclas Dec 5, 2017
@ghost ghost removed cla-already-signed labels Dec 5, 2017
@muratg
Copy link

muratg commented Oct 10, 2018

@MathyV Are you still interested in pursuing this change? Thanks.

@MathyV
Copy link
Contributor Author

MathyV commented Dec 8, 2018

Currently I'm on other projects so I'm not actively using your library. I'll check if I can find some time to fix the test but currently I have no need for it.

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

Successfully merging this pull request may close these issues.

4 participants