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

ignore nonfilesystem hrefs #162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luk4s
Copy link

@luk4s luk4s commented Aug 27, 2019

Hi,
we have situation that our outgoing mails contains some part of HTML page which contains a link to stylesheets in query string form

<link rel="stylesheet" href="?__debugger__=yes&amp;cmd=resource&amp;f=style.css" type="text/css">

Roadie raise error

TypeError: no implicit conversion of nil into String

in build_file_path method.

I change this method to prevent this exception. find_stylesheet! will raise CssNotFound when no real CSS file exist in href.

@codecov-io
Copy link

codecov-io commented Aug 27, 2019

Codecov Report

Merging #162 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #162      +/-   ##
==========================================
- Coverage   97.96%   97.96%   -0.01%     
==========================================
  Files          57       57              
  Lines        1963     1962       -1     
==========================================
- Hits         1923     1922       -1     
  Misses         40       40
Impacted Files Coverage Δ
spec/lib/roadie/filesystem_provider_spec.rb 97.95% <100%> (+0.18%) ⬆️
lib/roadie/filesystem_provider.rb 100% <100%> (ø) ⬆️
spec/integration_spec.rb 100% <0%> (ø) ⬆️
lib/roadie/asset_scanner.rb 100% <0%> (ø) ⬆️
lib/roadie/document.rb 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afda57d...c1d62e9. Read the comment docs.

@Mange
Copy link
Owner

Mange commented Aug 27, 2019

Is it possible to tag them with data-roadie-ignore instead? I'd prefer not to have this special case added, so I'd like to find a way around it first.

@luk4s
Copy link
Author

luk4s commented Aug 27, 2019

Unfortunately this HTML is not under my control.
Background: Our system receive text-plain message which contains HTML code with this style link. Message become from 3rd party blackbox.
We have some kind of alerting feature which mailing this "incident" to users - and in this case system failed on TypeError in roadie. I have covered exception in our alerting.

What about extend condition in lib/roadie/asset_scanner.rb:76 by checking format of value href attribute ? For example value can start only with http or / ?

@Mange
Copy link
Owner

Mange commented Aug 27, 2019

Would it work for you to add a before_transformation callable?

# Assuming plain roadie here; check roadie-rails's config if you are using that.
document.before_transformation = MarkInvalidStylesheetsAsIgnored.new
class MarkInvalidStylesheetsAsIgnored
  def call(dom, _document)
    dom.css('link[rel="stylesheet"][href^="?"]').each do |link|
      link["data-roadie-ignore"] = "data-roadie-ignore"
    end
  end
end

(Using a callable object to make it easier for you to unit test this.)

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.

3 participants