5
\$\begingroup\$

I've just recently started running some wargames (Natas, OverTheWire) and thought I'd implement some of my scripting experience to solve them instead of doing it manually (though I did that first).

I've got this script:

import requests
from requests.auth import HTTPBasicAuth

def exploit(host, credentials):
    print(host)
    try:

        r = requests.get(host,
                         auth=HTTPBasicAuth(credentials[0],
                                            credentials[1]),
                         headers={'referer': 'http://natas5.natas.labs.overthewire.org/'}
                         )

        print([line for line in r.content.decode('ascii').split('\n') \
               if 'natas5' in line][0])

    except Exception as e:
        print(e)

def main():
    host = 'http://natas4.natas.labs.overthewire.org'
    credentials = ('natas4', 'Z9tkRkWmpt9Qr7XrR5jWRkgOU901swEZ')
    exploit(host, credentials)

if __name__ == '__main__':
    main()

I know it's a really basic script, but I'd really want to know if I'm following the right coding conventions and if there are ways of making a bit better. I've rarely used the requests library, so I'm still fairly new to it.

\$\endgroup\$
  • \$\begingroup\$ Please tell us what this code accomplishes, and what the inputs and inputs look like. Also change the title to describe what the code does — all questions here seek feedback. (See How to Ask.) \$\endgroup\$ – 200_success Jun 13 '17 at 14:42
  • \$\begingroup\$ Follow-up question \$\endgroup\$ – 200_success Jun 14 '17 at 4:05
3
\$\begingroup\$

I would start with the following improvements:

  • better naming - for instance, r is not a good variable name. Remember that code is much more often read than written
  • avoid hardcoding constants - define them as proper Python constants (these are not really constants, but more like a guideline)
  • you can use next() built-in function instead of a list comprehension and then getting the first filtered item
  • you can unpack the credentials tuple into the HTTPBasicAuth arguments
  • avoid having a bare except clause - catch more specific exceptions if needed

Improved code:

import requests
from requests.auth import HTTPBasicAuth


REFERER = 'http://natas5.natas.labs.overthewire.org/'
KEYWORD = 'natas5'


def exploit(host, credentials):
    """TODO: docstring here"""
    try:
        response = requests.get(host,
                                auth=HTTPBasicAuth(*credentials),
                                headers={'referer': REFERER})

        response_lines = response.content.decode('ascii').split('\n')
        return next((line for line in response_lines if KEYWORD in line), 'Keyword not found')
    except requests.RequestException as e:
        print(e)


def main():
    host = 'http://natas4.natas.labs.overthewire.org'
    credentials = ('natas4', 'Z9tkRkWmpt9Qr7XrR5jWRkgOU901swEZ')
    print(exploit(host, credentials))


if __name__ == '__main__':
    main()
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback! I've already configured my solution to the next natas with this in mind. Will definitely change natas4 too, when I'm done! :) What about my syntax in response? Do you think that looks fine? :/ \$\endgroup\$ – geostocker Jun 13 '17 at 14:53
  • \$\begingroup\$ @geostocker no problem. Do you mean the logic of parsing the response? Well, this is an HTML response and probably a proper way to deal with that would be to HTML-parse it..eg. with BeautifulSoup - locate the div with id="content" element and get it's text..thanks. \$\endgroup\$ – alecxe Jun 13 '17 at 14:56
  • \$\begingroup\$ Nono, not the logic. More so how it's written. I'm thinking about pep8 standards etc. :) Since all my code goes on github, which is publicly available for future employers I'm hoping to keep it as well-documented and written as possible! :) \$\endgroup\$ – geostocker Jun 13 '17 at 14:57
  • \$\begingroup\$ @geostocker in terms of PEP8, we are more or less good to go with my version. But, code organization wise, it's difficult to tell - this is a very small piece of code. We'll need to see if it will grow, where and how, if it is gonna be usable, understandable and readable or not. At this stage, it's probably too early to tell. \$\endgroup\$ – alecxe Jun 13 '17 at 15:03
  • \$\begingroup\$ Cool! Well I'm sure I don't need to do any more refactoring. All the solutions will be decoupled, so they won't share any sort of code between themselves. I might create a common interface with argparse to allow a user to get the passwords from one place. I'll have to see how I decide to solve it later. :) \$\endgroup\$ – geostocker Jun 13 '17 at 15:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.