#5791: Address errors found by lgtm.com #7403

sturmer · 2019-04-29T10:54:43Z

SUMMARY

The website LGTM.com has detected a few errors from static analysis. This patch contains a possible fix for the first of them ("URL redirection from remote source") that follows the website's own recommendation: Check the input against some known source before using it. I've checked that the dashboard ID provided actually exists in DB and if it doesn't I return a 404.
I plan to solve the other 4 issues labeled as "Error" but wanted to get some early feedback before moving on.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Not applicable.

TEST PLAN

I've written a unit test that can be run e.g. like:
tox -e py37 -- tests/access_tests.py:RequestAccessTests.test_dashboard_endpoint_malicious_redirect

ADDITIONAL INFORMATION

Has associated issue: Improve code quality based on LGTM.com feedback #5791
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

REVIEWERS

Please @mistercrunch as original reporter but feel free to re-assign as you see fit.

codecov-io · 2019-05-02T13:19:57Z

Codecov Report

Merging #7403 into master will increase coverage by 0.02%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #7403      +/-   ##
==========================================
+ Coverage   65.71%   65.74%   +0.02%     
==========================================
  Files         459      459              
  Lines       21983    21980       -3     
  Branches     2415     2413       -2     
==========================================
+ Hits        14446    14450       +4     
+ Misses       7416     7410       -6     
+ Partials      121      120       -1

Impacted Files	Coverage Δ
superset/connectors/druid/views.py	`68.18% <ø> (+0.43%)`	⬆️
superset/assets/src/explore/controls.jsx	`42.85% <ø> (ø)`	⬆️
superset/assets/src/explore/exploreUtils.js	`76.47% <ø> (+0.74%)`	⬆️
superset/tasks/cache.py	`76.72% <ø> (ø)`	⬆️
superset/data/tabbed_dashboard.py	`22.22% <ø> (-12.16%)`	⬇️
superset/data/energy.py	`100% <ø> (ø)`	⬆️
...sets/src/SqlLab/components/ScheduleQueryButton.jsx	`8.77% <0%> (ø)`	⬆️
...c/explore/components/controls/withVerification.jsx	`96.77% <100%> (ø)`	⬆️
superset/utils/core.py	`88.1% <100%> (ø)`	⬆️
superset/connectors/druid/models.py	`82.43% <100%> (+0.04%)`	⬆️
... and 4 more

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 ebb7fbc...2b0d317. Read the comment docs.

john-bodley · 2019-05-21T16:14:45Z

superset/connectors/druid/models.py

@@ -83,6 +84,8 @@ def __init__(self, name, field_names, function):
 class CustomPostAggregator(Postaggregator):
    """A way to allow users to specify completely custom PostAggregators"""
    def __init__(self, name, post_aggregator):
+        # TODO(gianluca): Is this the right way? should I use the arg post_aggregator?
+        Postaggregator.__init__(self, None, None, name)


Please use super instead.

john-bodley · 2019-05-21T16:15:21Z

superset/stats_logger.py

@@ -82,6 +82,7 @@ def __init__(self, host='localhost', port=8125,
            If statsd_client argument is given, all other arguments are ignored and the
            supplied client will be used to emit metrics.
            """
+            BaseStatsLogger.__init__(self, prefix)


Please use super instead.

john-bodley · 2019-05-21T16:16:59Z

superset/utils/core.py

    def __get__(self, obj, objtype):
        if not self.is_method:
-            self.is_method = True
+            copy = self.__copy__()
+            return functools.partial(copy.__call__, obj)


Could you provide some more context as to what you’re trying to achieve here?

The analyzer at lgtm.com finds this assignment harmful (https://lgtm.com/rules/910074/).

The problem is (AFAIU) that we are mutating a descriptor object, and this can cause surprises to different instances of the same class. The recommendation is to create a copy that contains the desired state and return that instead.

In a patch I haven't pushed yet, I do a copy.is_method = True before returning as I do here. I wanted to copy the present instance, mutate it, and return it.

This, however, makes the unit tests test_memoized_on_methods_with_watches and test_memoized_on_methods fail.

On further thought, I think that the code uses this pattern to actually achieve the desired result. What I propose, in this case, is to either silence the alert with # lgtm[py/mutable-descriptor], or to just ignore it. Maybe a comment in the code would be enough?

@john-bodley kindly when you find a minute can you advise on this?

john-bodley · 2019-05-21T16:18:37Z

tests/access_tests.py

@@ -428,6 +428,14 @@ def test_approve(self, mock_send_mime):
            session.delete(security_manager.find_role(TEST_ROLE_NAME))
            session.commit()

+    def test_dashboard_endpoint_malicious_redirect(self):
+        # todo(gianluca.ciccarelli@bolt.eu): I need a dashboard ID that will


You could try using -1 (though I’m unsure if that works), otherwise maybe querying for the maximum dashboard ID and adding one, or using 9999.

mistercrunch · 2019-05-22T05:07:50Z

superset/connectors/druid/models.py

@@ -71,6 +71,7 @@ def _fetch_metadata_for(datasource):

 class JavascriptPostAggregator(Postaggregator):
    def __init__(self, name, field_names, function):
+        Postaggregator.__init__(self, function, field_names, name)


Edits to this file seems unrelated to your PR title

I agree. The thing is that I started on the issue but gave a title related to the first I had started tackling. Do you suggest changing the title or split the PR into a few ones? (I'd favor the first.)

sturmer · 2019-05-28T07:37:18Z

@mistercrunch @john-bodley please when you have time can you have a look at this? In the meanwhile, I'll start addressing the lower-priority alerts (I remember seeing many unused imports, they should be safe to remove).

sturmer · 2019-05-28T07:38:09Z

To be clear, I think this patch could be merged as-is, but I'll add some lower-priority work to it that won't hurt. At least that's the plan 😄

sturmer · 2019-06-21T08:25:51Z

I need to re-scope this one in light of the more recent alerts. I think that the important is that I manage to fix the 4 errors currently pointed out.

sturmer · 2019-06-21T08:38:09Z

Now it should fix the 4 Errors and tackle a few Warnings as a bonus. It would be useful to point LGTM.com to this branch and check that that is actually the case, but I haven't been able to.

Is there still interest in going through with this? @john-bodley @mistercrunch

…d ID

stale · 2019-08-25T11:15:56Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

pull-request-size bot added the size/S label Apr 29, 2019

sturmer force-pushed the 5791-improve-code-quality-based-on-lgtm_com-1 branch from 0ab7e72 to a3eada7 Compare May 2, 2019 13:04

sturmer force-pushed the 5791-improve-code-quality-based-on-lgtm_com-1 branch from 3146e1d to 7bb26da Compare May 20, 2019 06:38

john-bodley reviewed May 21, 2019

View reviewed changes

mistercrunch reviewed May 22, 2019

View reviewed changes

sturmer force-pushed the 5791-improve-code-quality-based-on-lgtm_com-1 branch from 7bb26da to 03ff19c Compare May 22, 2019 07:45

sturmer changed the title ~~[WIP] #5791: Return 404 when requesting access to unexisting dashboard ID~~ [WIP] #5791: Address errors found by lgtm.com May 22, 2019

sturmer force-pushed the 5791-improve-code-quality-based-on-lgtm_com-1 branch from 93cfe38 to e9c0a75 Compare May 24, 2019 13:35

sturmer force-pushed the 5791-improve-code-quality-based-on-lgtm_com-1 branch 2 times, most recently from e71dcf4 to e96416c Compare May 29, 2019 15:19

sturmer changed the title ~~[WIP] #5791: Address errors found by lgtm.com~~ #5791: Address errors found by lgtm.com Jun 21, 2019

pull-request-size bot added size/M and removed size/S labels Jun 21, 2019

sturmer force-pushed the 5791-improve-code-quality-based-on-lgtm_com-1 branch 2 times, most recently from 82df7c7 to 28acf04 Compare June 21, 2019 08:22

sturmer added 10 commits June 26, 2019 11:45

apache#5791: Return 404 when requesting access to unexisting dashboar…

d1b1094

…d ID

5791: Remove unused variables/imports

c9fab71

5791: Call parent class' __init__ in child's __init__ method

979840b

5791: Fix lint issues

6ea76bd

5791: Fix mutation of descriptor in __get__

1f52a29

Start addressing the reviewers' comments

c7f78cd

Set is_method in copy instance

6d596d1

Split long line

190520b

Fix call to super

6167d3e

Try correct indent

44a8d1d

sturmer added 6 commits June 26, 2019 11:47

Silence the alert

b22fd24

Address warning-level issues

23ca06c

Address more warnings

bc6fba0

Remove linter errors

06a68c8

Remove file introduced by mistake, call static method correctly

a14dee9

Remove todo

2b0d317

sturmer force-pushed the 5791-improve-code-quality-based-on-lgtm_com-1 branch from cd59f46 to 2b0d317 Compare June 26, 2019 08:49

stale bot added the inactive Inactive for >= 30 days label Aug 25, 2019

stale bot closed this Sep 1, 2019

snyk-bot mentioned this pull request Feb 18, 2023

[Snyk] Upgrade deck.gl from 8.5.2 to 8.8.23 yannhowe/superset#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#5791: Address errors found by lgtm.com #7403

#5791: Address errors found by lgtm.com #7403

sturmer commented Apr 29, 2019

codecov-io commented May 2, 2019 •

edited

Loading

john-bodley May 21, 2019

john-bodley May 21, 2019

john-bodley May 21, 2019

sturmer May 22, 2019

sturmer May 24, 2019 •

edited

Loading

sturmer May 28, 2019

john-bodley May 21, 2019

mistercrunch May 22, 2019

sturmer May 22, 2019

sturmer commented May 28, 2019

sturmer commented May 28, 2019

sturmer commented Jun 21, 2019

sturmer commented Jun 21, 2019

stale bot commented Aug 25, 2019

#5791: Address errors found by lgtm.com #7403

#5791: Address errors found by lgtm.com #7403

Conversation

sturmer commented Apr 29, 2019

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

codecov-io commented May 2, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sturmer May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sturmer commented May 28, 2019

sturmer commented May 28, 2019

sturmer commented Jun 21, 2019

sturmer commented Jun 21, 2019

stale bot commented Aug 25, 2019

codecov-io commented May 2, 2019 •

edited

Loading

sturmer May 24, 2019 •

edited

Loading