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

Fix URL extension parsing #1602

Merged
merged 2 commits into from Jul 29, 2017

Conversation

Projects
None yet
5 participants
@molikuner
Copy link
Contributor

molikuner commented Jul 13, 2017

when there is a querystring Pi-hole sometimes parsed a wrong extension

By submitting this pull request, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your PR:

Please submit all pull requests against the development branch. Failure to do so will delay or deny your request

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

3


fixing the bug that if the url has a querystring the extrension was wrong parsed
fixes bug of issue #1598

This template was created based on the work of udemy-dl.

Fix URL extension parsing
when there is a querystring Pi-hole sometimes parsed a wrong extension
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jul 13, 2017

CLA assistant check
All committers have signed the CLA.

@PromoFaux

This comment has been minimized.

Copy link
Member

PromoFaux commented Jul 15, 2017

Also, checking out this branch reminded me. Any changes to the block pages are not reflected on a users system during pihole -r or pihole -up, only on a fresh install.

I thought I had made a PR for fixing that, but I can't seem to find it....

@@ -21,8 +21,9 @@ function validIP($address){
}
// Retrieve server URI extension (EG: jpg, exe, php)
// strtok($uri, '\?') splits the querystring from the path (if there is a querystring)

This comment has been minimized.

Copy link
@PromoFaux

PromoFaux Jul 16, 2017

Member

Was this not working before because it wasn't actually setting a variable anywhere? If that's the case... 1/2

ini_set('pcre.recursion_limit',100);
$uriExt = pathinfo($uri, PATHINFO_EXTENSION);
$uriExt = pathinfo(strtok($uri,'\?'), PATHINFO_EXTENSION);
// Define which URL extensions get rendered as "Website Blocked"
$webExt = array('asp', 'htm', 'html', 'php', 'rss', 'xml');

This comment has been minimized.

Copy link
@PromoFaux

PromoFaux Jul 16, 2017

Member

2/2... does this fix now render this line unnecessary?

From my testing, I tried with http://adfoc.us/serve/sitelinks/?id=ANID&url=google.aspx (a valid URL extension not on the list!). Before I applied this change, I got the blank block page, and afterwards I got the full block page.

Seems to make sense to me, but @WaLLy3K may want to confirm as block page is his baby!

This comment has been minimized.

Copy link
@WaLLy3K

WaLLy3K Jul 17, 2017

Collaborator

http://adfoc.us/serve/sitelinks/?id=ANID&url=google.aspx is designed to be invalid, as anything with a ? query string is broadly determined to be a file, rather than attempting further detection.

That is to say, that's how I originally intended for it to be - I'm not 120% familiar with the current Block Page derivative as I am with my own.

@PromoFaux

This comment has been minimized.

Copy link
Member

PromoFaux commented Jul 29, 2017

Approved.

Through testing, this proposed PR appears to do what it says on the tin, and have not come across any oddities as yet.

Approved with PullApprove

@Mcat12 Mcat12 changed the base branch from development to release/3.2 Jul 29, 2017

@Mcat12

This comment has been minimized.

Copy link
Member

Mcat12 commented Jul 29, 2017

Approved
(changed to target the release branch)

@PromoFaux

This comment has been minimized.

Copy link
Member

PromoFaux commented Jul 29, 2017

Approved (again)

@Mcat12 Mcat12 merged commit 898fdf7 into pi-hole:release/3.2 Jul 29, 2017

4 checks passed

codacy/pr Good work! A positive pull request.
Details
code-review/pullapprove Approved by all reviewer groups.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@molikuner molikuner deleted the molikuner:development branch Oct 7, 2017

@jacobsalmela jacobsalmela referenced this pull request Dec 8, 2017

Closed

No blocked-page #1598

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.