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

add field to change font color #4879

Merged
merged 9 commits into from
Aug 7, 2019
Merged

add field to change font color #4879

merged 9 commits into from
Aug 7, 2019

Conversation

octavioamu
Copy link
Contributor

Description

Add field to change font color in hackathon explorer

Refers/Fixes

https://gitcoincore.slack.com/archives/CJWQWG6J2/p1564417276108500

Testing

with no selection
image

image
image

@codecov
Copy link

codecov bot commented Jul 29, 2019

Codecov Report

Merging #4879 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4879      +/-   ##
==========================================
+ Coverage    30.7%   30.71%   +<.01%     
==========================================
  Files         216      216              
  Lines       17429    17430       +1     
  Branches     2364     2364              
==========================================
+ Hits         5352     5353       +1     
  Misses      11862    11862              
  Partials      215      215
Impacted Files Coverage Δ
app/dashboard/models.py 56.46% <100%> (+0.02%) ⬆️

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 ac0248e...1728b2e. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 29, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@b4315d0). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4879   +/-   ##
=========================================
  Coverage          ?   30.65%           
=========================================
  Files             ?      217           
  Lines             ?    18972           
  Branches          ?     2822           
=========================================
  Hits              ?     5816           
  Misses            ?    12886           
  Partials          ?      270
Impacted Files Coverage Δ
app/dashboard/models.py 56.46% <100%> (ø)

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 b4315d0...7992401. Read the comment docs.

thelostone-mc
thelostone-mc previously approved these changes Jul 29, 2019
danlipert
danlipert previously approved these changes Jul 30, 2019
Copy link
Contributor

@danlipert danlipert left a comment

Choose a reason for hiding this comment

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

Looks great! Just one possible change to the help text - or not, either way :)

@@ -3268,7 +3268,8 @@ class HackathonEvent(SuperModel):
logo_svg = models.FileField(blank=True)
start_date = models.DateTimeField()
end_date = models.DateTimeField()
background_color = models.CharField(max_length=255, null=True, blank=True, help_text='hexcode for the banner')
background_color = models.CharField(max_length=255, null=True, blank=True, help_text='hexcode for the banner, default to white')
Copy link
Contributor

Choose a reason for hiding this comment

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

For the help text here, do we want to make a note about html colors also being allowed as in your example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, just people using hex will be fine. I don't know if the user will know the entity right names also.

<div id="{{ hackathon.identifier }}" class="row pt-5 pb-5 text-center banner"
{% if hackathon.background_color %} style="background: {{ hackathon.background_color }}" {% endif %}
>
<div id="{{ hackathon.identifier }}" class="row pt-5 pb-5 text-center banner" style="background: {% firstof hackathon.background_color or 'white' %}; color: {% firstof hackathon.text_color or 'black' %};">
Copy link
Contributor

Choose a reason for hiding this comment

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

In the files above I have not seen any verification about the fact that the entered data is actually a color. How will it be verified if the entered data is not a fake string of characters that can perform a CSRF attack or break out of the default css styles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But is actually only a backend field accessible by admin. Do you think is a problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

Think about it differently.

Your approach is normal, but a bit dangerous. If all fields in the admin panel are created in this way, it means that if someone finds 1 vulnerability (administrative access) then he will be able to use some new vulnerabilities by interfering with the infrastructure. However, if the administrator's panel fields is also properly secured, someone who wrongly gains access will have less opportunities to abuse it.

Not securing something because access is only available to administrators may in the future cause a lot of problems. Increases the risk of escalating 1 bug to critical levels with the possibility of using other bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with your point but also this is only called in an inline style tag in html. So even being an open field and anyone having access I think will not open any hack opportunity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, maybe you're right. I'm just sensitive to reporting such things. I am a bit of a pedant and that I deal with security, how do I see something like that I report :D

Seeing that I composed in my head automatically a potential route of attack from past experience

Input via color field: black; background-image('URL');

I was able to create a CSRF vulnerability, where in inline html I was able to put a marker that downloads a given url. And then perform administrative actions from the user level by including the appropriate url in it.

thelostone-mc
thelostone-mc previously approved these changes Aug 7, 2019
@octavioamu octavioamu dismissed stale reviews from thelostone-mc and danlipert via ffb3826 August 7, 2019 14:54
@danlipert danlipert merged commit 0ddfa05 into master Aug 7, 2019
@thelostone-mc thelostone-mc deleted the hackathon-font-color branch June 27, 2020 00:44
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.

4 participants