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

auto_link shouldn't always sanitize #6495

Merged
merged 1 commit into from
May 26, 2012
Merged

auto_link shouldn't always sanitize #6495

merged 1 commit into from
May 26, 2012

Conversation

homakov
Copy link
Contributor

@homakov homakov commented May 26, 2012

sanitize is not always required so we cannot force it by default. let's just
whitelist protocols and everybody's happy

My commit was wrong - I didn't realize how it will impact on raw text.

@tenderlove also should I bump gem version of rails_autolink ? It's bugfix so I just pushed it to the 1.0.8

sanitize is not always required so we cannot make it. let's just
whitelist protocols
@homakov
Copy link
Contributor Author

homakov commented May 26, 2012

Figured out - Repushing of gem versions is not allowed. OK bumped to 1.0.9

Now URL's like sasdfasdf://something.com will be no longer autolinked! And I think it's cool - nobody knows all dangerous protocols(browsers can add some) like javascript: and whitelist will save from it..

@arunagw
Copy link
Member

arunagw commented May 26, 2012

cc @tenderlove

@rafaelfranca
Copy link
Member

Would ne great if you add some test. But I'm merging this since the build is broken.

rafaelfranca added a commit that referenced this pull request May 26, 2012
auto_link shouldn't always sanitize
@rafaelfranca rafaelfranca merged commit 5989ffb into rails:3-0-stable May 26, 2012
@homakov
Copy link
Contributor Author

homakov commented May 26, 2012

@rafaelfranca yes I'd love to add tests. I should probably push into rails_autolink since it's no longer in 3.1

@rafaelfranca
Copy link
Member

The build is still broken. http://travis-ci.org/#!/rails/rails/jobs/1442465. Please take a look.

Could you at least run the tests before send the pull request?

@homakov
Copy link
Contributor Author

homakov commented May 26, 2012

It's not broken!


1) Failure:
115test_auto_link_other_protocols(TextHelperTest) [/home/vagrant/builds/rails/rails/actionpack/test/template/text_helper_test.rb:454]:
116<"<a href=\"file:///home/username/RomeoAndJuliet.pdf\">file:///home/username/RomeoAndJuliet.pdf</a>"> expected but was
117<"file:///home/username/RomeoAndJuliet.pdf">.

We do not support file:/// URLs anymore - we should remove that testcase

@rafaelfranca
Copy link
Member

Ok. Please remove it. 👍

@tenderlove
Copy link
Member

@homakov yes, you should release a new rails_autolink with your fixes. Thanks!

@homakov
Copy link
Contributor Author

homakov commented May 26, 2012

@rafaelfranca Yes in that comment I implied you to do it - it's just 1 line, I don't want to pollute issue trackers with tiny pull requests:) Anyway I can do PR if you wish

@tenderlove yes, I figured it out, new gem is released

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.

None yet

4 participants