Why is this cycle endless?
So, I just got my site from today and I think this feature is the culprit. Can anyone tell me what the problem is? I can't seem to figure it out:
Public Function CleanText(ByVal str As String) As String
'removes HTML tags and other characters that title tags and descriptions don't like
If Not String.IsNullOrEmpty(str) Then
'mini db of extended tags to get rid of
Dim indexChars() As String = {"<a", "<img", "<input type=""hidden"" name=""tax""", "<input type=""hidden"" name=""handling""", "<span", "<p", "<ul", "<div", "<embed", "<object", "<param"}
For i As Integer = 0 To indexChars.GetUpperBound(0) 'loop through indexchars array
Dim indexOfInput As Integer = 0
Do 'get rid of links
indexOfInput = str.IndexOf(indexChars(i)) 'find instance of indexChar
If indexOfInput <> -1 Then
Dim indexNextLeftBracket As Integer = str.IndexOf("<", indexOfInput) + 1
Dim indexRightBracket As Integer = str.IndexOf(">", indexOfInput) + 1
'check to make sure a right bracket hasn't been left off a tag
If indexNextLeftBracket > indexRightBracket Then 'normal case
str = str.Remove(indexOfInput, indexRightBracket - indexOfInput)
Else
'add the right bracket right before the next left bracket, just remove everything
'in the bad tag
str = str.Insert(indexNextLeftBracket - 1, ">")
indexRightBracket = str.IndexOf(">", indexOfInput) + 1
str = str.Remove(indexOfInput, indexRightBracket - indexOfInput)
End If
End If
Loop Until indexOfInput = -1
Next
End If
Return str
End Function
source to share
Wouldn't that be easier? (OK, I know it is not identical to the posted code):
public string StripHTMLTags(string text)
{
return Regex.Replace(text, @"<(.|\n)*?>", string.Empty);
}
(Converting to VB.NET should be trivial!)
Note. If you do this frequently, there are two performance improvements you can make to Regex
.
One is to use a precompiled expression that requires rewriting.
The second is to use the non-capturing form of the regular expression; .NET Regular Expressions implement the (? :) syntax that allows grouping without sacrificing the performance of the captured text, which is remembered as a back reference. Using this syntax, the above regex can be changed to:
@"<(?:.|\n)*?>"
source to share
This line is also wrong:
Dim indexNextLeftBracket As Integer = str.IndexOf("<", indexOfInput) + 1
It is guaranteed to always set indexNextLeftBracket equal to indexOfInput, because at this point the character at the position referenced by indexOfInput is already always '<'. Do this instead:
Dim indexNextLeftBracket As Integer = str.IndexOf("<", indexOfInput+1) + 1
And also add a clause to the if statement to make sure your string is long enough for this expression.
Finally, as others have said, this code is going to be a beast to maintain, if you can get it to work at all. Better to look for another solution like regex or even just replace all '<' with <
.
source to share
In addition to other good answers, you can read a little about loop invariants . Pulling and dropping stuff onto the line you are checking to end the loop should include all beeps. :)
source to share
Easy to guess, but is it like a criminal? indexOfInput = str.IndexOf (indexChars (i)) 'find an instance of indexChar
In Microsoft Docs , the Return Value is the Index position value if this string is found, or -1 if it is not. If the value is Empty, the return value is 0.
So maybe indexOfInput is set to 0?
source to share
What happens if your code tries to clear a line <a
?
As I read it, it finds indexChar at position 0, but then indexNextLeftBracket and indexRightBracket are 0, you hit the else clause and then you insert ">" at position -1, which is supposedly inserted at the beginning giving you a string ><a
. The new indexRightBracket then becomes 0, so you remove from position 0 for 0 characters while leaving ><a
. Then the code will find it <a
in the code again , and you will go to race with an infinite loop of memory consumption.
Even if I'm wrong, you need to get some unit tests to reassure yourself that these edge cases are working correctly. This should also help you find the actual loop code if I'm out of base.
In general, though, even if you fix this particular bug, it will never be very reliable. Parsing HTML files is tricky and HTML blacklists will always have holes. For example, if I really want to get a tag <input type="hidden" name="tax"
, I'll just write it as <input name="tax" type="hidden"
and your code will ignore it. Your best bet is to use an actual HTML parser and only allow the (very small) subset of tags that you really want. Or better yet, use a different form of markup and strip all HTML tags (again using a real HTML parser of some description).
source to share
I would have to run it through a real compiler, but mindpiler tells me that the line will str = str.Remove(indexOfInput, indexRightBracket - indexOfInput)
re- generate an invalid tag such that when it loops again it finds the same error "fixes" it, tries again, finds the error "fixes" it, and so on.
FWIW is a piece of code that removes unwanted HTML tags from a string (this is in C #, but the concept is translated)
public static string RemoveTags( string html, params string[] allowList )
{
if( html == null ) return null;
Regex regex = new Regex( @"(?<Tag><(?<TagName>[a-z/]+)\S*?[^<]*?>)",
RegexOptions.Compiled |
RegexOptions.IgnoreCase |
RegexOptions.Multiline );
return regex.Replace(
html,
new MatchEvaluator(
new TagMatchEvaluator( allowList ).Replace ) );
}
MatchEvaluator class
private class TagMatchEvaluator
{
private readonly ArrayList _allowed = null;
public TagMatchEvaluator( string[] allowList )
{
_allowed = new ArrayList( allowList );
}
public string Replace( Match match )
{
if( _allowed.Contains( match.Groups[ "TagName" ].Value ) )
return match.Value;
return "";
}
}
source to share
It doesn't work for a simplified case <a<a<a
or even <a>Test</a>
. Have you tested this at all?
Personally, I hate string parsing like this, so I'm not going to even try to figure out where your error is. This will require a debugger and more headache than I am willing to contribute.
source to share