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 a protocol to relative urls in FileHandler fixes #4918 #4957

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

octfx
Copy link
Contributor

@octfx octfx commented Mar 25, 2021

This PR is made in Reference to #4918

Tries to add a protocol to a relative url, by checking wgCanonicalServer or the request through RequestContext::getMain. If both fail http is used as a fallback.

@kghbln
Copy link
Member

kghbln commented Mar 30, 2021

@octfx Thanks for providing this patch and please excuse that is has been rotting here since.

@JeroenDeDauw Could you have a peep at this?

@JeroenDeDauw
Copy link
Member

I'm afraid to merge this without tests

@octfx
Copy link
Contributor Author

octfx commented Apr 27, 2021

I'll try to add some when my time permits it!

@26
Copy link

26 commented May 19, 2021

I do not think this would work for two reasons:

  • file_get_contents also does not understand protocol-less URIs. The URL also needs to be protocolized before it is passed to that.
  • In the function protocolizeUrl, you return the unaltered canonical URL if it has a scheme. This means any path or query that was passed to the function would be lost.

I believe just removing lines 136 to 145 (the try-catch block) would fix the second issue. The first issue may be fixed by assigning the protocolized URL to a variable and passing that variable to both file_get_contents as well as get_headers.

@octfx
Copy link
Contributor Author

octfx commented May 19, 2021

I do not think this would work for two reasons:

  • file_get_contents also does not understand protocol-less URIs. The URL also needs to be protocolized before it is passed to that.
  • In the function protocolizeUrl, you return the unaltered canonical URL if it has a scheme. This means any path or query that was passed to the function would be lost.

I believe just removing lines 136 to 145 (the try-catch block) would fix the second issue. The first issue may be fixed by assigning the protocolized URL to a variable and passing that variable to both file_get_contents as well as get_headers.

Oh, the file_get_contents part is a good catch, that slipped through!

I've updated the try-catch block to add the canonical server protocol to the input url.

Further I've added some tests, but I couldn't get them to run locally as it always errored out with Use of undefined constant SMW_PHPUNIT_AUTOLOADER_FILE [...] boostrap.php on line 15 :/

- Add FileIndexerTest
@JeroenDeDauw
Copy link
Member

@Seb35 does this look good to you?

@marijnvanwezel
Copy link
Contributor

@JeroenDeDauw Could you take a look at this again when you have the time? We are facing this issue again.

@JeroenDeDauw
Copy link
Member

@marijnvanwezel did you test that this patch fixes the issue?

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.

None yet

5 participants