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

OpenURL #318

Closed
wants to merge 2 commits into from
Closed

OpenURL #318

wants to merge 2 commits into from

Conversation

geoffthemedio
Copy link
Member

Adds a function to open a specified URL with the system default web browser. Does some basic checking to ensure the URL is actually a website.

Also adds a button the intro screen that should open freeorion.org.

Would like to know if it works for other OSes, and if not, how to fix it.

@Cjkjvfnby
Copy link
Contributor

You did not mention that you use Windows 8.1

Does that means that it is possible to have pedia in html format and open it from game. Some articles as game guide and game concepts will be more easy to read via html (I hate small text in small windows).

@Dilvish-fo
Copy link
Member

I'm rather surprised it works fine for you-- freerion.org does not appear to me to support https, and none of my browsers in Linux will automatically change https to http (seems that would be a security flaw if they did).

Aside from that, it seems to work ok for me in Ubuntu. It does not bring the browser to the forefront, but then that doesn't happen if I open a commit link from within SmartGit either.

@geoffthemedio
Copy link
Member Author

What is the relevance of the specific version of Windows I use?

Only static pages in the pedia could be opened in a separate window, and then links between that and universe-specific pages wouldn't work.

@Cjkjvfnby
Copy link
Contributor

| What is the relevance of the specific version of Windows I use?

Because I need to browse forum to check what do you mean by works for other OSes

@MatGB
Copy link
Member

MatGB commented Aug 29, 2015

He's got a point Geoff, Win10 is significantly different in a variety of ways, the default browser is new for a start, it'll need testing in each OS and possibly a variety of browser defaults in case one of them is a bit hinky.

@@ -1127,3 +1128,44 @@ HumanClientApp* HumanClientApp::GetApp()

void HumanClientApp::Initialize()
{}

void HumanClientApp::OpenURL(const std::string& url) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm 100% sure that there are trusted libraries for checking if a string is a valid URL, and 110% sure that there are more constraints on URLs than you've tested for here.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, after some searching, I can't find such a library, which is both astonishing and disconcerting... My second point still stands, though. We should probably at least use a regular expression to make sure it's a valid URL.

Copy link
Member Author

Choose a reason for hiding this comment

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

The point of the tests is to make sure it's not a link to a local file, so that it can't be used as a way to execute arbitrary code locally if used from within a pedia link (not yet implemented). Whether it is actually a valid URL isn't so important as preventing it from being something else potentially malicious. If the URL doesn't resolve or isn't valid after passing these tests, then that isn't an engine problem, probably.

If you want to add a much more complicated test, feel free to suggest something, but I think these tests serve my purpose.

@Vezzra Vezzra added the category:feature The Issue/PR describes or implements a new game feature. label Aug 30, 2015
@Vezzra Vezzra added this to the Next release milestone Aug 30, 2015
@Vezzra
Copy link
Member

Vezzra commented Aug 30, 2015

Would like to know if it works for other OSes

Aside from the issue Dilvish mentioned:

I'm rather surprised it works fine for you-- freerion.org does not appear to me to support https, and none of my browsers in Linux will automatically change https to http (seems that would be a security flaw if they did).

...this works well on OSX. I had to change the hardcoded link to http://..., otherwise I get the same problem Dilvish described.

@geoffthemedio geoffthemedio deleted the OpenURL branch September 1, 2015 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:feature The Issue/PR describes or implements a new game feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants