-
Notifications
You must be signed in to change notification settings - Fork 14k
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
Allowing config flag to turn off javascript controls #4400
Allowing config flag to turn off javascript controls #4400
Conversation
mistercrunch
commented
Feb 11, 2018
fee79d3
to
1c48352
Compare
1c48352
to
4297559
Compare
4297559
to
93d0625
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, one question only: have you considered completely removing the form in this case, instead of having the warning? Can we also disable the Javascript form in this case, so it's read only?
superset/views/core.py
Outdated
d = cast_form_data(request.args) | ||
d = cast_form_data(d) | ||
|
||
for k in FORM_DATA_KEY_BLACKLIST: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: you can do this in a single line
d = {k: v for k, v in d.items() if k not in FORM_DATA_KEY_BLACKLIST}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra points for avoiding the mutation
Since the default is to set the feature off (secure should be the default), I want for the users to see that the feature exists otherwise there's no way for the users to discover that the feature exists. I looked quickly at the react-ace to see if a |
Oh looks like there's a readOnly prop I missed on my first pass |
@betodealmeida made the fields |
For the record a security expert mentioned in an email:
|
Addressed comments, merging. |
* Allowing config flag to turn off javascript controls * lint * one line, avoiding mutation * Setting JS fields as readOnly (cherry picked from commit a373db2)
* Allowing config flag to turn off javascript controls * lint * one line, avoiding mutation * Setting JS fields as readOnly
* Allowing config flag to turn off javascript controls * lint * one line, avoiding mutation * Setting JS fields as readOnly
* Allowing config flag to turn off javascript controls * lint * one line, avoiding mutation * Setting JS fields as readOnly (cherry picked from commit a373db2)