fix(test): compare the whole line, so the link that routes reporters is actually covered - #314
Merged
Merged
Conversation
…is actually covered Code scanning flagged this assertion as incomplete URL substring sanitization, high severity. As a security finding it is a false positive - the string being tested is our own output, not attacker input. The alert was worth acting on anyway, because it pointed at a test that was weaker than it looked. The assertion checked for 'https://github.com/msgwing/ZeroSMTP/issues/new' while the code emits that URL plus '?template=error-string.yml'. Remove the parameter and the substring is still present, so the old test passed on broken output. That parameter is what routes a reporter to the structured error-string form rather than a blank issue. Now it compares whole trimmed lines for exact equality, which is stronger and no longer has the shape the scanner reads as a URL check. Verified by breaking it on purpose: deleting the parameter fails exactly one test, restoring it returns 25 passing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alert 15, waga wysoka:
js/incomplete-url-substring-sanitization.Jako znalezisko bezpieczeństwa — fałszywy alarm
Sprawdzany ciąg to nasz własny tekst wyjściowy, nie dane od atakującego.
Nie ma tu żadnej sanityzacji do obejścia. CodeQL widzi kształt
X.includes('https://...')i nie odróżnia go od sprawdzania adresu.Ale wskazał realną słabość testu
Asercja szukała
https://github.com/msgwing/ZeroSMTP/issues/new, a kod wypisujeten adres plus
?template=error-string.yml.Usuń parametr — fragment nadal tam jest, więc stary test przechodził na
zepsutym kodzie. A ten parametr kieruje zgłaszającego do strukturalnego
formularza zamiast do pustego zgłoszenia.
Poprawka
Porównanie całych przyciętych linii na dokładną równość. Mocniejsze niż
szukanie fragmentu i bez kształtu, który myli skaner.
Sprawdzone przez celowe zepsucie
Dokładnie jeden test się wywala. Bramka, której nie widziałem jak się wywala,
nie byłaby sprawdzona — to ten sam warunek, którego wymagałem od kontrybutora
przy #300, więc stosuję go do siebie.