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

SUPEE-7405 from Magento 1.9.3.0 - Hostname Validation #314

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sreichel
Copy link

The Zend_Validate_Hostname validation was updated to add a DNS check of the A record for the domain.

The Zend_Validate_Hostname validation was updated to add a DNS check of the A record for the domain.
}
$result = checkdnsrr($toAscii, 'A');
} else {
$idn = new Net_IDNA2();
Copy link

@boenrobot boenrobot Jan 19, 2023

Choose a reason for hiding this comment

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

Surely, at least "suggested" for "pear/Net_IDNA2" will need to be added for this. Also, this should probably be conditional.

Also, what if I don't have the Intl extension and I don't want to depend on "pear/Net_IDNA2" either? Maybe if neither Intl or Net_IDNA2 are present, fallback to giving checkdnsrr() the raw hostname?

Also, on a related note, I'm not sure I want my hostname validation to cause a DNS lookup altogether. What if I want to add a hostname that isn't currently resolvable (because it's a domain that will be launched later)? Or is internal to a different system than the one my code is running in? In both cases, I still want to validate that I've given a syntactically valid name that may possibly exist, but not necessarily one that truly exists. I think this entire change should be behind a new option that defaults to not running the check, for the sake of backwards compatibility. The currently failing tests on 8.0 are failing exactly because the test data for this validator is not an existing hostname, though still a syntactically valid one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@develart-projects
Copy link
Collaborator

@sreichel could you make that conditional? This should not be by default for sure.

@develart-projects develart-projects added enhancement New feature or request question Further information is requested requested change some changes are needed before the merge and removed question Further information is requested labels Aug 9, 2023
@develart-projects develart-projects added this to the 1.24.0 milestone Aug 10, 2023
@develart-projects develart-projects marked this pull request as draft August 23, 2023 12:51
@develart-projects develart-projects removed this from the 1.24.0 milestone Sep 27, 2023
@sreichel sreichel marked this pull request as ready for review January 25, 2024 23:39
@sreichel
Copy link
Author

sreichel commented Jan 26, 2024

@develart-projects i'm stuck. Is it me, or the tests? localhost.localdomain is valid when falling back to checkdnsrr() , not?

@boenrobot
Copy link

@develart-projects i'm stuck. Is it me, or the tests? localhost.localdomain is valid when falling back to checkdnsrr() , not?

checkdnsrr() does a DNS lookup. While the domain in the test suite is syntactically valid, it is not an actual hostname reachable by the test runner, so it fails.

@sreichel
Copy link
Author

sreichel commented Feb 2, 2024

  1. Zend_Validate_HostnameTest::testBasic
    localhost.localdomain
    Failed asserting that true matches expected false.

Need some advice how to fix it.

@boenrobot
Copy link

  1. Zend_Validate_HostnameTest::testBasic
    localhost.localdomain
    Failed asserting that true matches expected false.

Need some advice how to fix it.

Add an option. Default to not using it. When not using it, don't do your whole thing.

The current behavior, as is, already works for all syntactically valid hostnames. It's just that it accepts names that don't exist, which may or may not be what you need.

@sreichel
Copy link
Author

sreichel commented Feb 3, 2024

Add an option.

How should it look like? I cant change the isValid() methods signature (adding a parameter).

Add a new constant to turn it on/off?

@develart-projects
Copy link
Collaborator

imo you need to test both cases. Old one and your new update, so run 2 different tests for 2 different use cases.
I think you can simply feed some common domain, like google.com for testing.

@develart-projects develart-projects marked this pull request as draft October 14, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request requested change some changes are needed before the merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants