Skip to content

SONARPHP-1925 S1313: Fix FPs on test files and with almost-IPv4 strings - #1802

Merged
malte-skoruppa-sonarsource merged 2 commits into
masterfrom
gb/SONARPHP-1925
Sep 10, 2026
Merged

SONARPHP-1925 S1313: Fix FPs on test files and with almost-IPv4 strings#1802
malte-skoruppa-sonarsource merged 2 commits into
masterfrom
gb/SONARPHP-1925

Conversation

@guillem-bartrina-sonarsource

@guillem-bartrina-sonarsource guillem-bartrina-sonarsource commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Additionally, simplify the IP_V6 regex (NFC).


Summary by Gitar

  • Bug fixes:
    • Fixed false positives on test files and with almost-IPv4 strings in IPv4Check

This will update automatically on new commits.

@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

SONARPHP-1925

@guillem-bartrina-sonarsource guillem-bartrina-sonarsource changed the title SONARPHP-1925 Fix FPs on test files and with almost-IPv4 strings SONARPHP-1925 S1313: Fix FPs on test files and with almost-IPv4 strings Sep 9, 2026

@lijun-chen-sonarsource lijun-chen-sonarsource left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM!

Just left a small question.

@gitar-bot

gitar-bot Bot commented Sep 10, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 3 resolved / 3 findings

Fixes false positives in S1313 (hardcoded IP addresses) by excluding test files and refining regex patterns. Parameterized suppression test message and S1313 ruling expectations were corrected, and the IPv6 regex was simplified. No issues remain.

✅ 3 resolved
Quality: Parameterized suppression test hardcodes S6418 in its message

📄 sonar-php-plugin/src/test/java/org/sonar/plugins/php/PHPSensorTest.java:479-489
shouldBeSuppressedOnTestLikePath is now parameterized over S6418 and S1313, but the assertion description is still the literal "S6418 should be suppressed in tests/", so a failure of the S1313 case reports the wrong rule. The test also has no positive counterpart (unlike s2068ShouldRaiseIssueOnNonTestPath), so it would pass vacuously if S1313 ever stopped raising on testFileExcludedCheck/tests/HardCodedIpAddress.php at all; consider parameterizing a non-test-path variant that asserts hasSize(1).

Bug: Ruling expectations for S1313 still list issues under test/

📄 php-checks/src/main/java/org/sonar/php/checks/HardCodedIpAddressCheck.java:31
Making HardCodedIpAddressCheck implement TestFileExcludedCheck means AnalysisScanner.filterTestFileExcludedIssues drops S1313 issues for any file matching **/test/** (sonar.tests is not set in the ruling run — RulingHelper explicitly filters the "Test files were detected using a path heuristic" warning). its/ruling/.../expected/PHPMailer/php-S1313.json still expects issues at PHPMailer:test/PHPMailer/IsValidHostTest.php lines 54, 57, 58, 92, 99, so the LITS comparison in PhpGeneralRulingTest.testPhpMailer will report those 5 as missing and assertThat(litsDifferences).isEmpty() will fail. Delete that expectation file (the whole entry is a test path). Also re-check expected/monica/php-S1313.json (config/location.php:130) against the tightened IPv4 lookahead, since the regex now rejects matches followed by . or a word character.

Quality: IPv6 lookahead edit is a no-op and misses the trailing dot

📄 php-checks/src/main/java/org/sonar/php/checks/HardCodedIpAddressCheck.java:59 📄 php-checks/src/main/java/org/sonar/php/checks/HardCodedIpAddressCheck.java:39
(?![\d\w:])(?![\w:]) changes nothing, because \d is a subset of \w; I ran both patterns over the same inputs and every result was identical. Consequently the trailing-dot fix applied to IP_V4 was not applied to IP_V6, so "2003:db8:a0b:12f0::1." still raises while the analogous "1.2.3.4." no longer does (and IPv4-embedded IPv6 forms such as "::ffff:0:1.2.3.4." keep matching). Add \. to the IPv6 lookahead for consistency, or drop the no-op edit.

Implementation Status ✅ 2 of 2 objectives covered
SONARPHP-1925 - 2 of 2 objectives covered

This PR covers both objectives by preventing S1313 from being raised on test files and refining the regular expressions to avoid false positives on almost-IPv4 strings.

✅ 2 covered here
  • ✅ Do not raise S1313 on test files
  • ✅ Refine the regexes for S1313 to avoid false positives on almost-IPv4 strings
Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next

Copy link
Copy Markdown

@malte-skoruppa-sonarsource
malte-skoruppa-sonarsource merged commit de392a3 into master Sep 10, 2026
22 checks passed
@malte-skoruppa-sonarsource
malte-skoruppa-sonarsource deleted the gb/SONARPHP-1925 branch September 10, 2026 09:40
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.

4 participants