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

Add app version to http header of dynamic access link fetch #2067

Closed
wants to merge 1 commit into from
Closed

Add app version to http header of dynamic access link fetch #2067

wants to merge 1 commit into from

Conversation

cornzzy
Copy link

@cornzzy cornzzy commented Jul 25, 2024

env is undefined, I wasn't sure what's the best way to import it in this function.

This change has the following impacts:

  • Dynamic access link web service can now force client to update Outline if version is not supported
  • Makes it future proof if one day an Outline version is not backward compatible
  • Prevents detection by DPI if client using an old/bad Outline version

My test:
I put 50 clients with versions from 1.10.0 to 1.13.1 on a server and after a day it got detected and returned ServerUnreachable error.

Then I put 50 clients which I was sure they're using version 1.13 and 1.13.1 on windows and it's still going strong with no detection, it even has UDP enabled.

issue #2061

`env` is undefined, I wasn't sure what's the best way to import it in this function.

This change has the following impacts:
- Dynamic access link web service can now force client to update Outline if version is not supported
- Makes it future proof if one day an Outline version is not backward compatible
- Prevents detection by DPI if client using an old/bad Outline version


My personal test:
I put 50 clients with versions from `1.10.0` to `1.13.1` on a server and after a day it got detected and returned `ServerUnreachable` error.

Then I put 50 clients which I was sure they're using version 1.13 and 1.13.1 on windows and it's still going strong with no detection, it even has UDP enabled.
@cornzzy cornzzy requested a review from a team as a code owner July 25, 2024 03:14
@sbruens
Copy link
Contributor

sbruens commented Jul 25, 2024

Hi @cornzzy,

Thanks for this PR. Unfortunately, we can't merge this.

The User-Agent header would be a better location to specify this information. We should add it there instead of creating a new header we'd have to maintain forever.

Having said that, adding it to the User-Agent header is a bit of a pain right now given how we use both Electron and Cordova. We have some upcoming changes to fetch the config in the Go backend, which will make it easier to add version and OS to the User-Agent in a single place. We can specify it in the header there, which will resolve your issue.

/cc @jyyi1 who is leading the work on moving the config fetching to Go

@fortuna
Copy link
Collaborator

fortuna commented Nov 6, 2024

FYI, I believe the fetch in Go has been merged for all platforms already, so this change could be reimplemented in the Go code.

/cc @jyyi1

@jyyi1
Copy link
Contributor

jyyi1 commented Nov 7, 2024

FYI, I believe the fetch in Go has been merged for all platforms already, so this change could be reimplemented in the Go code.

/cc @jyyi1

Please approve the updated PR (after rebase) #2222 when you have time. Thanks.

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

Successfully merging this pull request may close these issues.

4 participants