Skip to content

Commit

Permalink
closed XSS after the @
Browse files Browse the repository at this point in the history
  • Loading branch information
bcherry committed Aug 23, 2010
1 parent b2b27bc commit cffce8e
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 1 deletion.
2 changes: 1 addition & 1 deletion lib/regex.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class Regex
# Allow @ in a url, but only in the middle. Catch things like http://example.com/@user
REGEXEN[:valid_url_path_chars] = /(?:
#{REGEXEN[:wikipedia_disambiguation]}|
@[^\/]+\/|
@#{REGEXEN[:valid_general_url_path_chars]}+\/|
[\.\,]?#{REGEXEN[:valid_general_url_path_chars]}
)/ix
# Valid end-of-path chracters (so /foo. does not gobble the period).
Expand Down
8 changes: 8 additions & 0 deletions spec/autolinking_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,14 @@ def original_text; "I like www.foobar.com dudes"; end
end
end

context "with a @ in a URL" do
def original_text; 'http://x.xx/@"style="color:pink"onmouseover=alert(1)//'; end

it "should not allow XSS follwing @" do
@autolinked_text.should have_autolinked_url('http://x.xx/')
end
end

end

describe "Autolink all" do
Expand Down

10 comments on commit cffce8e

@romac
Copy link

@romac romac commented on cffce8e Sep 21, 2010

Choose a reason for hiding this comment

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

This would have prevented today's XSS exploits on Twitter.com.
Why haven't this commit been pulled ?

@robinduckett
Copy link

Choose a reason for hiding this comment

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

Hah.

@timparker
Copy link

Choose a reason for hiding this comment

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

Ha indeed!

@bitemyapp
Copy link

Choose a reason for hiding this comment

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

Easy to criticize when you're not in the trenches guys...

@1stvamp
Copy link

Choose a reason for hiding this comment

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

Even easier to criticise when you have been in similar trenches and it still looks bad. ;-)

@bitemyapp
Copy link

Choose a reason for hiding this comment

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

when you have been in similar trenches and it still looks bad. ;-)

I've been in that exact situation of being able to criticize because I do know better, but people are a little eager to dogpile the 'big guy' like Twitter.

@romac
Copy link

@romac romac commented on cffce8e Sep 21, 2010

Choose a reason for hiding this comment

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

No one has to be criticized, this kind of things can happen. 100% bug-free is not possible. I'm just wondering why this hasn't been pulled by Twitter ?

@bitemyapp
Copy link

Choose a reason for hiding this comment

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

Deployment processes and code-vetting are non-instantaneous.

@hoverbird
Copy link
Collaborator

Choose a reason for hiding this comment

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

The fix is in production now.

@mathiasbynens
Copy link

Choose a reason for hiding this comment

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

@romac: This was in fact patched last month. Read the Twitter blog:

We discovered and patched this issue last month. However, a recent site update (unrelated to new Twitter) unknowingly resurfaced it.

Please sign in to comment.