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

Adds django linter violations to xss linter. #1821

Merged
merged 1 commit into from
Jul 25, 2019

Conversation

adeelkhan
Copy link
Contributor

This adds documentation for django linter violations that is added in https://github.com/edx/edx-platform/pull/19615

LEARNER-4632

@adeelkhan adeelkhan requested a review from robrap April 30, 2019 12:16
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Thanks @adeelkhan. This looked really good. I figured it would be simpler to make my comments as changes. Please review my changes and ensure I didn't introduce any typos or issues.

I made the following changes:

  • I used start_link instead of start_anchor, etc., so the names make more sense to translators.
  • I update spacing.
  • I removed new lines in long text to match our newer spec for documentation.
  • I update the examples with a url and switched to some_url to make it shorter. Please ensure I did this correctly.
  • Various other edits.

Thank you!


.. code::

## DO this
Copy link
Contributor

Choose a reason for hiding this comment

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

@adeelkhan: What would happen if any of these that use interpolate_html were mistakenly also wrapped with {% filter force_escape %}? Will the linter find that and complain? Is it a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now linter would not complain, but i think this can be added. I dont think it is correct to use both and any developer would use them simultaneously, but i think django template would not complain too. The question is we can use a tag inside a filter in django templates. I have to check that.

The idea is linter inspects between blocktrans/endblocktrans content to see if there is any html/interpolated variable inside it or not. Based on it decides
1) If foudn regular string, move up and treat it as regular blocktrans tag and find filter expression for such block.
2) If found any html content, flag it as violation, we would not allow this.
3) If found any interpolation variable eg {var}, then look for connected interpolate_html expression. It connects them using output variable used like in below example variable is tmsgx

So there are three cases:

  {% blocktrans trimmed asvar tmsgx %}
      {{span_start}} hour{% plural %}{{span_end}} hours
  {% interpolate_html tmsgx span_start='<span>'|safe span_end='</span>'|safe %}
  {% endblocktrans %}
{% endfilter %}

Linter would complain as it finds html tags inside blocktrans/endblocktrans, so it doesnt checks if its under filter/endfilter block. We would not allow html usage inside blocktrans/endblocktrans.

  {% blocktrans trimmed asvar tmsgx %}
    {{span_start}} hour{% plural %}{{span_end}} hours
  {% endblocktrans %}
  {% interpolate_html tmsgx span_start='<span>'|safe span_end='</span>'|safe %}
{% endfilter %}

Now as there is no html/interpolate variable inside blocktrans/endblocktrans, linter would expect it as a regular block and move up to find {% filter force_escape %} expression.

  {% blocktrans trimmed asvar tmsgx %}
      {span_start} hour{% plural %}{span_end} hours
  {% endblocktrans %}
  {% interpolate_html tmsgx span_start='<span>'|safe span_end='</span>'|safe %}
{% endfilter %}

Linter finding interpolation variable like {span_start} would try to find related interpolate_html and further checks it has the correct arguments and any html used is passed through safe filter.

There is no check on the position of interpolate_tag. I guess a blocktrans/endblocktrans could be in one position and its not necessary for its interpolate_html tag to be aligned with it. It can be anywhere in file.

@adeelkhan adeelkhan force-pushed the adeel/django_template_linter_docs branch from c40a0bc to 296ec82 Compare May 2, 2019 07:38
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Thanks.

@adeelkhan adeelkhan merged commit 04255d9 into master Jul 25, 2019
@adeelkhan adeelkhan deleted the adeel/django_template_linter_docs branch July 25, 2019 19:05
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.

2 participants