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

Bugfix for issue#102 -- "5.11 breaks HTML::FormatExternal" #103

Closed
wants to merge 11 commits into from

Conversation

Perlbotics
Copy link

Test case added.
Skipping attempt to unescape empty authority part.

See: #102

Test case added.
Skipping attempt to unescape empty authority part.

See: libwww-perl#102
@Perlbotics
Copy link
Author

Hi Olaf (@oalders), seems that an entry is missing in Changes?
Can I edit this file myself or is that task performed by a tool like dzil somewhere upstream?

@oalders
Copy link
Member

oalders commented Jul 7, 2022

That can be done manually so feel free to add the entry. 😀

@Perlbotics
Copy link
Author

Thank you, Olaf (@oalders). Changes updated and pushed.

Changes Outdated Show resolved Hide resolved
lib/URI.pm Outdated Show resolved Hide resolved
t/file.t Outdated
@@ -63,3 +63,13 @@ for my $t (@tests) {
print "ok $testno\n";
$testno++;
}


{ #-- https://github.com/libwww-perl/URI/issues/102 -- "5.11 breaks HTML::FormatExternal"
Copy link
Member

Choose a reason for hiding this comment

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

This could also be

Regression test for #102.

Copy link
Author

Choose a reason for hiding this comment

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

It should be added, that I was able to validate this test on Linux only.
It can be added to the generic test data structure in the beginning of t/file.t
if the results for Windows and MacOs have been verified.

@oalders oalders requested a review from simbabque July 7, 2022 17:05
@Perlbotics Perlbotics requested a review from oalders July 7, 2022 18:35
Perlbotics added 2 commits July 8, 2022 09:14
The previous fix checked the result of the regex-match.
However, the regex-match could have avoided the situation
in the first place.

The new regex now asks for a non-zero authority part.

Details: libwww-perl#102
Perlbotics added 2 commits July 9, 2022 12:06
Currently: data, file, ldapi, urn, sqlite, sqlite3
Short test cases added for 'mailto:' URIs having
address literals (IPv4 and IPv6).
@Perlbotics
Copy link
Author

Perlbotics commented Jul 9, 2022

Hi Olaf (@oalders), I identified schemes that do not have an authority part and thus do not need a special treatment
of the host part (768e8e6). Early return for these. Not quite necessary, but I think the intention is cleaner now.

The second patch (84176ec) is more important. 'mailto:' has a complex format that needs special treatment.
Since I have no solution for now, the previous 5.10 encoding is restored.
I also added test cases to t/mailto.t that would have helped to detect this bug earlier.

Please consider to publish the corrections or to revert to 5.10 since the current bug (file:///) might
have security implications or could cause havoc on the file system.

Update: regression tests in t/file.t were only verified for Linux. See #103 (comment)

Copy link
Member

@oalders oalders left a comment

Choose a reason for hiding this comment

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

Thanks for this @Perlbotics! Just a few stylistic changes requested. Thank you for being thorough with the added tests.

t/file.t Outdated Show resolved Hide resolved
t/file.t Outdated Show resolved Hide resolved
t/file.t Outdated Show resolved Hide resolved
lib/URI.pm Outdated Show resolved Hide resolved
lib/URI.pm Outdated Show resolved Hide resolved
t/file.t Outdated Show resolved Hide resolved
lib/URI.pm Show resolved Hide resolved
@Perlbotics Perlbotics requested a review from oalders July 10, 2022 16:39
Copy link
Contributor

@simbabque simbabque left a comment

Choose a reason for hiding this comment

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

Looks good to me. Nice work, thank you.

@oalders
Copy link
Member

oalders commented Jul 10, 2022

Thanks for being a good sport through all of these iterations @Perlbotics! I've merged this manually into master as 725fbfb.

Closes #102.

@oalders oalders closed this Jul 10, 2022
@Perlbotics
Copy link
Author

Thank you Olaf (@oalders), Julien (@simbabque), and Graham (@haarg) for your patience and support.
This was the first time, I participated in a github patch activity and while finding great support here, I also learned a lot.

BTW: If you want to improve test coverage, try to run everything once more with something like:
URI_HAS_RESERVED_SQUARE_BRACKETS=1 prove -l
I've done this manually here w/o problems but perhaps it is worth an extra author test?

Thanks.

@oalders
Copy link
Member

oalders commented Jul 11, 2022

This was the first time, I participated in a github patch activity and while finding great support here, I also learned a lot.

Congratulations! 🎊 I'm glad it was a positive experience.

If you want to improve test coverage, try to run everything once more with something like:
URI_HAS_RESERVED_SQUARE_BRACKETS=1 prove -l

We can do this in the GitHub workflow, which is at https://github.com/libwww-perl/URI/blob/master/.github/workflows/dzil-build-and-test.yml

Maybe add a job which needs: build-job and just runs the tests again on Linux using Perl 5.36. I don't think we necessarily want to run the test suite twice for the entire matrix. If we just add that one extra run, I think that would be good enough.

@simbabque
Copy link
Contributor

Maybe add a job which needs: build-job and just runs the tests again on Linux using Perl 5.36. I don't think we necessarily want to run the test suite twice for the entire matrix. If we just add that one extra run, I think that would be good enough.

@Perlbotics if you need help with this, give us a shout. I'm happy to explain how these actions work. :)

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

3 participants